-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Fix disabled menu item button styles #16581
Conversation
da4589d
to
63d2574
Compare
Thanks for catching this Dan! I was trying to think about where else this opacity change might occur and didn't even consider this one. Thanks for the fix! 👍 I tested this PR, and it works as expected. Let's get this in. |
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.
.components-menu-item__button { | ||
&:disabled, | ||
&[aria-disabled="true"] { | ||
opacity: 0.3; |
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.
Was this fixed in a more global way @aduth? Should we revert that change in that case as it becomes unnecessary styles?
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.
Yes, this treats a symptom of the root issue which is that, as of #16103 (#16103 (comment)), we have no disabled stying for buttons.
I think the changes there (at least this one line) should be reverted, or find another way which treats exceptions to default disabled styling.
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.
See #16769
Description
This is a small fix for a visual regression introduced in #16103.
In that PR an opacity of 0.3 that was applied to any disabled button was removed. This caused disabled menu items to no longer appear disabled. This screenshot shows a menu with some enabled and some disabled menu items when that opacity is no longer present:
This PR reintroduces that opacity style, but only for menu items:
How has this been tested?
Expected (in this branch)
Buttons in the dropdown menu appear greyed out.
Actual (in master)
Buttons in the dropdown menu appear active, but cannot be clicked on or interacted with
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist: