-
-
Notifications
You must be signed in to change notification settings - Fork 80
Fix expander #412
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
base: main
Are you sure you want to change the base?
Fix expander #412
Conversation
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.
Pull request overview
This PR addresses a bug where the expander control fails to expand after recent changes. The fix includes three main modifications: restructuring the event handler registration logic to only initialize state when the animation feature is enabled, adding a conditional check in the event handler to handle cases where animations are disabled, and explicitly setting visibility before animating expansion.
Key Changes
- Refactored event handler registration to occur outside the
IsEnabledcheck, moving initialization logic inside the conditional block - Added
IsEnabledcheck inOnExpanderExpandedOrCollapsedto handle visibility directly when animations are disabled - Explicitly set
Visibility.Visiblebefore starting expand animation to ensure proper state
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expander.Expanded += OnExpanderExpandedOrCollapsed; | ||
| expander.Collapsed += OnExpanderExpandedOrCollapsed; |
Copilot
AI
Dec 17, 2025
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.
Event handlers are registered unconditionally regardless of whether IsEnabled is true or false. This means handlers will be attached even when the feature is disabled, and they will be registered multiple times if the property is set repeatedly. This can lead to memory leaks and unexpected behavior. The handlers should only be registered when IsEnabled is true, and should be unregistered when IsEnabled changes to false.
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.
@copilot open a new pull request to apply changes based on this feedback
source/iNKORE.UI.WPF.Modern/Controls/Helpers/ExpanderAnimationsHelper.cs
Outdated
Show resolved
Hide resolved
…sHelper.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
NotYoojun
left a comment
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.
Currently an Expander with IsExpanded="True" in XAML will throw an exception:
<Expander Style="{StaticResource {x:Static ui:ThemeKeys.ExpanderCardStyleKey}}" Header="Welcome" HorizontalAlignment="Stretch" IsExpanded="True">
<Expander.Content>
<Grid HorizontalAlignment="Stretch" Background="Red">
<TextBlock HorizontalAlignment="Right">Test</TextBlock>
</Grid>
</Expander.Content>
</Expander>
Not sure if this is introduced here or originally existing. Perhaps we can merge this after its fixed.
In the version currently released on NuGet, when animation is disabled, the expander doesn't expand at all.
After a recent fix for the initial expander state (blinking), expansion doesn't work at all (with or without animation).
This fix fixes this issue.