Dialog template back. Added test to check dialog title is shown #197
Conversation
Added test to check dialog title is shown
📝 WalkthroughWalkthroughThis pull request introduces several changes across multiple files related to the The Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/Tests/Consolonia.Gallery.Tests/DialogTests.cs (1)
20-20: LGTM! Consider adding a descriptive comment.The new assertion correctly verifies that the dialog title is shown. To improve test readability, consider adding a comment explaining the test flow: navigation -> dialog open -> title verification -> dialog close.
await UITest.KeyInput(Key.Tab); await UITest.KeyInput(Key.Enter); await UITest.KeyInput(Key.Tab); await UITest.AssertHasText("One More"); + // Verify dialog title is shown await UITest.AssertHasText(SomeDialogWindow.DialogTitle); await UITest.KeyInput(Key.Escape);src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (2)
1-8: LGTM! Good architectural improvementThe change from
ResourceDictionarytoStylesas the root element is a better architectural choice for control templates, following Avalonia's recommended patterns for control styling.
Line range hint
97-104: Consider using a resource for content marginsThe content presenter's margin values (2,1,2,1) are hardcoded. Consider moving these to a resource for better maintainability and consistency.
+ <Styles.Resources> + <Thickness x:Key="ConsoloniaDialogWindowContentMargin">2,1,2,1</Thickness> + </Styles.Resources> - <Panel Margin="2,1,2,1"> + <Panel Margin="{DynamicResource ConsoloniaDialogWindowContentMargin}">
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml(0 hunks)src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs(1 hunks)src/Consolonia.Themes/Templates/Controls/AllControls.axaml(1 hunks)src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml(2 hunks)src/Tests/Consolonia.Gallery.Tests/DialogTests.cs(2 hunks)
💤 Files with no reviewable changes (1)
- src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
🔇 Additional comments (4)
src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml.cs (1)
10-11: LGTM! Clean implementation of dialog title.
Good use of a constant for the dialog title, making it reusable across the class and tests while maintaining encapsulation with the internal modifier.
Also applies to: 17-17
src/Consolonia.Themes/Templates/Controls/AllControls.axaml (1)
46-46: Verify impact on other dialog windows.
The change from MergeResourceInclude to StyleInclude aligns with the template structure. However, let's verify this doesn't affect other dialog windows in the codebase.
✅ Verification successful
Change from MergeResourceInclude to StyleInclude is correct and safe
The verification confirms this change is appropriate because:
- The DialogWindow.axaml file contains Styles as its root element, making StyleInclude the correct inclusion method
- Other dialog windows (SomeDialogWindow.axaml) reference DialogWindow as a control, not as a resource
- TurboVisionDark theme extends the base DialogWindow style properly using ControlTheme
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other dialog window implementations that might be affected
rg -l "DialogWindow" --type xaml
Length of output: 61
Script:
#!/bin/bash
# Search for dialog window files and usages with regular extension
rg -l "DialogWindow" --type xml
# Find all XAML files in the repository
fd -e axaml
Length of output: 5588
Script:
#!/bin/bash
# Let's check the content of relevant dialog-related files
cat src/Consolonia.Core/Controls/Dialog/DialogWrap.axaml
cat src/Consolonia.Gallery/Gallery/GalleryViews/SomeDialogWindow.axaml
cat src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml
cat src/Consolonia.Themes/TurboVisionDark/Controls/DialogWindow.axaml
Length of output: 9480
src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml (2)
Line range hint 23-47: LGTM! Well-structured visual implementation
The layered approach using multiple panels with proper z-indexing creates a clean console-style UI. The consistent use of ConsoloniaDialogWindowBorderMargin resource ensures uniform spacing.
Line range hint 76-81: Verify title binding with programmatic title setting
The template correctly binds to the Title property, but let's verify it works with the programmatic title setting mentioned in the PR summary.
✅ Verification successful
Let me gather more information about the title binding implementation in the template.
Title binding implementation is correctly configured and working
The verification confirms that:
- The template correctly binds to Title using
{TemplateBinding Title}in the TextBlock - The sample dialog (SomeDialogWindow) demonstrates proper programmatic title setting
- The title appears in a StackPanel alongside the icon with appropriate layout
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Title property usage in dialog windows
rg -t cs "Title\s*=\s*DialogTitle" --type-add "cs:*.{cs,axaml}"
# Search for any other title-related bindings
rg -t cs "Title\s*=\s*" --type-add "cs:*.{cs,axaml}" -g "**/*DialogWindow*.{cs,axaml}"
Length of output: 490
Script:
#!/bin/bash
# Search for Title property in DialogWindow template
rg -t cs "Title" -A 5 -B 5 src/Consolonia.Themes/Templates/Controls/DialogWindow.axaml
# Search for title-related properties in DialogWindow class
ast-grep --pattern 'class DialogWindow {
$$$
Title
$$$
}'
Length of output: 908
No description provided.