Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add Expander Events #9555
Add Expander Events #9555
Changes from 6 commits
12d8fe7
adf5613
c5b8483
307142e
149aae7
dd60ea4
38a52fb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is kind of code I generally would prefer to avoid, as this API changes the property value without actually setting the value in conventional way.
Although, it also seems to the most convenient way to do these events for the users who would use this control.
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.
I tend to agree with you. I made sure to document reasoning well in case there are different suggestions. However, while it looks bad in general, I can't think of any downsides. It should be perfectly harmless. It's just one of those bad special cases.
That said, I did think of a way around this. It requires removing the toggle button entirely from the control template. State would then be managed entirely within the Expander itself -- which is probably a fundamentally better design to begin with. I think WPF was also being lazy and didn't want to duplicate a lot of code, as well.
Im not sure how much code duplication would be required to remove the toggle button from the default template. Pointer press and click would have to be entirely managed by the Expander. Keyboard navigation as well. It would probably be similar to what we did for SplitButton.
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.
I'll leave this open and we can always come back and change things in the future. It shouldn't be a bad breaking change except for those writing their own control themes (rare).
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.
It probably will be less customizable without the toggle button