-
-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix dialog title #245
Fix dialog title #245
Conversation
add border chars around it to make it look nicer.
📝 WalkthroughWalkthroughThe pull request introduces changes to two distinct areas of the Consolonia project. In the In the Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/Consolonia.Core/Text/Esc.cs (1)
47-53
: Consider adding usage documentation for the newly introduced mouse-tracking constants.While these new constants are correctly specified ANSI escape codes, it would be valuable to include short doc comments explaining their intended usage scenarios and potential terminal compatibility caveats. This can help maintainers or contributors easily grasp their functionality.
src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (4)
45-46
: Consider consolidating into a single grid definition if feasible.
Currently, we have a nested<Grid ColumnDefinitions="*">
containing another<Grid ColumnDefinitions="* Auto">
right beneath it. If there's no specific need for two grid layers, you may simplify the layout into a single grid definition to reduce nesting and keep the XAML more maintainable.
51-53
: Check for font compatibility with special Unicode characters.
The added "╡" and "╞" might not render in all fonts or terminals. Make sure they appear correctly under the project's supported environments.
56-71
: Optional: add a tooltip for the close button.
Providing a tooltip (e.g., "Close") can improve accessibility and clarity for users unfamiliar with the "×" icon.
74-86
: Fallback strategy for double-line borders.
If some environments do not correctly render double-line characters, consider a fallback or a property-based toggle to handle graceful degradation.Do you want me to propose a fallback mechanism using style triggers?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Consolonia.Core/Drawing/DrawingContextImpl.cs
(1 hunks)src/Consolonia.Core/Text/Esc.cs
(1 hunks)src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml
(1 hunks)
🔇 Additional comments (2)
src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (1)
47-54
: Verify the data-binding forIsVisible
on the Icon.
TheIsVisible="{Binding Icon.Length}"
binding expects a boolean, whereasIcon.Length
is numeric. For reliability, consider a converter binding (e.g., "Icon.Length > 0") to ensure a proper boolean is supplied.src/Consolonia.Core/Drawing/DrawingContextImpl.cs (1)
162-162
: Confirm intended behavior of defaulting to a new pen.
Previously, anull
pen meant no stroke. Now, you create a default pen with thickness 1. This changes the drawing logic, potentially introducing unintended strokes. Ensure this new behavior aligns with design goals.
Co-authored-by: <[email protected]>
@@ -159,8 +159,7 @@ public void DrawGeometry(IBrush brush, IPen pen, IGeometryImpl geometry) | |||
// if we have strokes to draw | |||
if (streamGeometry.Strokes.Count > 0) | |||
{ | |||
if (pen == null || pen.Thickness == 0) | |||
return; | |||
pen = pen ?? new Pen(brush); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the case when pen is not passed?
==| text |===