refactor: NavigationViewEx property change logic and simplify control#780
refactor: NavigationViewEx property change logic and simplify control#780
Conversation
dosubot
bot
added
the
size:L
There was a problem hiding this comment.
Pull request overview
This PR refactors the NavigationViewEx control to consolidate dependency property change callbacks and improve code maintainability. The changes follow an established pattern in the codebase (as seen in ErrorInfo control) and simplify the property change handling logic.
Changes:
- Consolidated all dependency property change callbacks into a single
OnPropertyChangedmethod with property equality checks - Removed the unused
SettingsItemStyledependency property and related code - Improved content grid animation translation offset calculation to use actual dimensions instead of hardcoded values
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| Screenbox/Controls/NavigationViewEx.cs | Implements consolidated property change handler, removes SettingsItemStyle usage, updates animation offset calculation to use actual grid dimensions, and improves overlay initialization logic |
| Screenbox/Controls/NavigationViewEx.Properties.cs | Replaces individual property change callbacks with single consolidated callback and removes SettingsItemStyle property definition |
| { | ||
| _contentGrid.Visibility = visibility; | ||
| _contentGrid.Visibility = ContentVisibility; | ||
| UpdateContentGridAnimations(); |
There was a problem hiding this comment.
UpdateContentGridAnimations() is now called every time ContentVisibility changes (via line 372), which is a new behavior introduced by this refactoring. Previously, it was only called during OnApplyTemplate() and when ContentAnimationDirection changed. Since the animations themselves don't depend on the current visibility state (only on ContentAnimationDirection), calling this method on every visibility change may be unnecessary and could impact performance if visibility changes frequently. Consider whether this call should be removed from UpdateContentVisibility() and only kept in the ContentAnimationDirectionProperty change handler.
| UpdateContentGridAnimations(); |
There was a problem hiding this comment.
It ensures we get the correct dimensions whenever visibility changes.
| // We use the ContentGrid LightDismissLayer rectangle fill to avoid tracking | ||
| // LightDismissOverlayMode, theme and high contrast changes ourselves. | ||
| if (_splitView?.FindDescendant<Rectangle>(r => r.Name == "LightDismissLayer") is Rectangle contentRootRect) | ||
| if (_splitView?.FindDescendant<Rectangle>(r => r.Name.Equals("LightDismissLayer", StringComparison.Ordinal)) is { } contentRootRect) |
There was a problem hiding this comment.
Condition is always not null because of ... != ....
| if (property == OverlayProperty) | ||
| { | ||
| LoadOverlay(); | ||
| } | ||
| else if (property == OverlayZIndexProperty) | ||
| { | ||
| UpdateOverlayZIndex(); | ||
| } | ||
| else if (property == ContentVisibilityProperty) | ||
| { | ||
| UpdateContentVisibility(); | ||
| UpdateOverlayLayout(); | ||
| } | ||
| else if (property == BackButtonStyleProperty) | ||
| { | ||
| UpdateBackButtonStyle(); | ||
| UpdateCloseButtonStyle(); | ||
| } | ||
| else if (property == PaneSearchButtonStyleProperty) | ||
| { | ||
| UpdatePaneSearchButtonStyle(); | ||
| } | ||
| else if (property == ContentAnimationDirectionProperty) | ||
| { | ||
| UpdateContentGridAnimations(); | ||
| } |
There was a problem hiding this comment.
This is a perfect opportunity for a switch statement.
There was a problem hiding this comment.
I did try it, but it wasn't the most readable block of code. I can give it another go.
There was a problem hiding this comment.
Honestly, the if/else chain seems like the cleaner and more straightforward solution, and it's a clear improvement over the previous implementation. I can't seem to find a switch statement syntax that feels satisfying.
rename animation direction property
SettingsItemStyledependency propertyWithout StyleTypedPropertyAttribute
Gravacao.2026-03-05.135312.mp4
With StyleTypedPropertyAttribute
Gravacao.2026-03-05.135125.mp4