Skip to content

Fix disabled menu item button styles#16581

Merged
talldan merged 2 commits into
masterfrom
fix/disabled-menu-item-styling
Jul 18, 2019
Merged

Fix disabled menu item button styles#16581
talldan merged 2 commits into
masterfrom
fix/disabled-menu-item-styling

Conversation

@talldan
Copy link
Copy Markdown
Contributor

@talldan talldan commented Jul 15, 2019

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:

Screen Shot 2019-07-15 at 11 33 55 am

This PR reintroduces that opacity style, but only for menu items:
Screen Shot 2019-07-15 at 11 36 45 am

How has this been tested?

  1. Add a table block to a post
  2. Without selecting a cell, click on the edit toolbar button on the edit toolbar

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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@talldan talldan added [Type] Bug An existing feature does not function as intended [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Component] Button labels Jul 15, 2019
@talldan talldan self-assigned this Jul 15, 2019
@talldan talldan added this to the Gutenberg 6.2 milestone Jul 15, 2019
@talldan talldan added the Needs Design Feedback Needs general design feedback. label Jul 15, 2019
@talldan talldan force-pushed the fix/disabled-menu-item-styling branch from da4589d to 63d2574 Compare July 15, 2019 04:50
@mapk
Copy link
Copy Markdown
Contributor

mapk commented Jul 15, 2019

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.

@mapk mapk removed the Needs Design Feedback Needs general design feedback. label Jul 15, 2019
Copy link
Copy Markdown
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks good! Left a comment but it's more of a question.
Fwiw this is what it looks like in high contrast mode on Windows:

disabled_buttons

(not sure if there's a better way of indicating disabled status for low vision users though)

Comment thread packages/components/src/dropdown-menu/style.scss Outdated
@talldan talldan merged commit 5a9ab0a into master Jul 18, 2019
@talldan talldan deleted the fix/disabled-menu-item-styling branch July 18, 2019 08:13
.components-menu-item__button {
&:disabled,
&[aria-disabled="true"] {
opacity: 0.3;
Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #16769

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants