Skip to content
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

Replaced "MenuGroup" with "NavigableMenu" in Warning Component. (Fixed:#12503) #12522

Merged
merged 3 commits into from
Jan 18, 2019

Conversation

iamdharmesh
Copy link
Contributor

Description

Replaced "MenuGroup" with "NavigableMenu" in Warning Component. which get rid of menu group title "More options" and fixes #12503

How has this been tested?

Screenshots

editor_warning_popover

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution @iamdharmesh the code and tests look good to me.

Before:
screenshot 2018-12-03 at 15 03 04

After:
screenshot 2018-12-03 at 15 14 40

@jorgefilipecosta jorgefilipecosta added this to the 4.8 milestone Dec 3, 2018
@gziolo gziolo added the [Type] Enhancement A suggestion for improvement. label Dec 14, 2018
@gziolo gziolo requested a review from afercia December 14, 2018 15:05
@@ -40,13 +40,13 @@ function Warning( { className, actions, children, secondaryActions } ) {
/>
) }
renderContent={ () => (
<MenuGroup label={ __( 'More options' ) }>
<NavigableMenu orientation="vertical" className="editor-warning-menu__content">
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like a lower-level component and API-wise it seems better to have a MenuGroup wrapping MenuItems which makes me wonder if we can't just support no passing a label to MenuGroup instead of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@youknowriad Thanks for reviewing. Yes, you are right. better to use MenuGroup without label as NavigableMenu is lower-level component. I will make this change soon.

@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@afercia
Copy link
Contributor

afercia commented Dec 19, 2018

@youknowriad a MenuGroup without label will produce a NavigableMenu with an aria-labelledby attribute that points to nothing. That's against the ARIA spec, will be reported by accessibility automated tools as an error, and might confuse assistive technologies. (Edit: to be more clear: it's basically equivalent to an empty label element).

Also, I can understand high-level argumentations but I don't see the need to have a wrapping <div> element in the DOM that doesn't do anything.

I'd say either the MenuGroup should be changed to not render an aria-labelledby attribute if the "label" element it points to is absent, or the NavigableMenu menu should be made a full citizen "high level" component.

@youknowriad
Copy link
Contributor

I'd say either the MenuGroup should be changed to not render an aria-labelledby attribute if the "label" element it points to is absent

This is what I had on my mind. People shouldn't have to think too much to understand how to use a given component.

@iamdharmesh
Copy link
Contributor Author

@youknowriad I have made aria-labelledby rendering conditional. can you please check.

@youknowriad
Copy link
Contributor

Code wise, this looks good. Can we get another check after the recent changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Warning component should use a normal Menu instead of MenuGroup
5 participants