-
Notifications
You must be signed in to change notification settings - Fork 711
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
[SplitButton] Add missing SplitButton disabled state and resource bindings #6138
[SplitButton] Add missing SplitButton disabled state and resource bindings #6138
Conversation
Also, I noticed that the disabled SplitButton does not have the same background color as a disabled Button assuming that SplitButtonBackgroundDisabled is the correct brush for that. Is that intentional? |
@tashatitova and @marksfoster and @chigy the split button previously did not have a disable state so the SplitButtonBackgroundDisabled brush wasn't being used. I don't think its been updated and needs to be updated to match the ButtonBackgroundDisabled value. Do you agree? |
@StephenLPeters , @tashatitova and @marksfoster , I would imagine it will take the same color as the disabled button here so if it is not the same color today, I'd imagine it would need to be updated. |
@chingucoding could you make that update as a part of this change? :) |
@StephenLPeters @chigy agreed on disabled color |
Aaannndddd done @StephenLPeters! SplitButton is quite strange in how some parts need to be transparent since otherwise multiple partially opaque brushes would result in a view different to the desired result. |
yeah, it has a strange, and probably incorrect architecture |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
@chingucoding can you update the before and after screenshots? Can we see the high contrast as well? |
Updated the screenshots now, SplitButton definitely could use some template refactoring, adding those brushes was definitely harder than one might think @StephenLPeters . |
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
Description
The disabled visualstate was missing on SplitButton, this is fixed now. The
UpdateVisualState
function now also takes theIsEnabled
property into account and skips other states if the control is disabled. In addition to that, the disabled background and disabled border brushes are now being respected.Motivation and Context
Closes #6011
How Has This Been Tested?
Tested manually.
Screenshots (if appropriate):