Components: Add `info` prop support to MenuItem, FeatureToggle #10802

aduth · 2018-10-19T14:32:34Z

This pull request seeks to add a new info prop to the MenuItem and FeatureToggle components.

Testing Instructions:

Verify an extended description is shown for the More Menu Unified Toolbar button, as in the screenshot above:

Notable variations:

Checked, unchecked
Cross-browser

Ensure unit tests pass:

npm run test-unit

aduth · 2018-10-19T15:52:56Z

packages/components/src/menu-item/style.scss


-	// Don't wrap text until viewport is beyond the mobile breakpoint.


This change directly undoes the intention of #8756.

Guess it depends if we'd rather:

Wrapped Unwrapped

Also, the comment is misleading, where the previous behavior had the opposite effect.

cc @jasmussen @tofumatt

@alexislloyd thoughts on this?

Maybe worth noting currently there is a max-width of the menu (480px), after which the text simply becomes clipped.

I think wrapped is better here, as long as we're careful not to have the descriptions go over two lines with standard font sizes.

I'm also ok with editing down the first description to "Access all tools in one place"

We also have the option to enforce a max-width (with wrapping) on the description specifically, allowing both the main label and shortcut text grow unrestrained.

Edit Actually, this might not work so well, or at least it leaves the possibility that the description is prematurely wrapped when the button has a long primary label.

aduth · 2018-10-19T15:55:19Z

test/e2e/specs/links.test.js

-		} else {
-			await page.click( '.edit-post-more-menu button' );
-		}
+	const toggleFixedToolbar = async ( isFixed ) => {


cc @noisysocks re #8807 (comment)

youknowriad · 2018-10-19T16:04:17Z

packages/components/src/icon-button/index.js

@@ -41,7 +41,7 @@ class IconButton extends Component {
 		);

 		let element = (
-			<Button { ...additionalProps } aria-label={ label } className={ classes }>
+			<Button aria-label={ label } { ...additionalProps } className={ classes }>


Can't you just use the label prop?

Not sure what you mean by "use". Button doesn't have a label prop.

youknowriad · 2018-10-19T16:06:08Z

packages/components/src/menu-item/README.md

+- Type: `string`
+- Required: No
+
+Refer to documentation for [Shortcut's `shortcut` prop](../shortcut/README.md#shortcut).


Thanks for the detailed README here 👍

youknowriad · 2018-10-19T16:12:33Z

Design questions

Can't we leave the shortcuts there somehow?
Can we reduce the "opacity" or change a little bit the color of the description (to separate it more clearly from the title) (I think opacity: 0.84; is the minimum opacity to be accessible)

youknowriad · 2018-10-19T16:12:52Z

Code wise, this look good 👍

aduth · 2018-10-19T16:19:03Z

Can't we leave the shortcuts there somehow?

Which shortcuts?

Can we reduce the "opacity" or change a little bit the color of the description

We already have two other variations of a descriptive subtext, one in the Post Visibility menu, the other of sidebar Controls (e.g. Drop Cap description). As implemented here, I've inherited precisely the style used for the sidebar controls (as proposed at #10340 (comment) and seconded at #10340 (comment)).

Now, that being said, I kinda agree with you 😄 But I didn't want to go ahead and add a third variation of what amounts to the same usage.

youknowriad · 2018-10-19T16:21:58Z

@aduth I was confused, just noticed these modes don't have shortcuts, I thought they had.

youknowriad · 2018-10-19T16:23:03Z

I think it was suggested elsewhere but maybe the solution here is to darken the default color of the links instead.

youknowriad · 2018-10-19T16:23:49Z

I think there's a snapshot that needs updating (e2e tests)

youknowriad

LGTM 👍 (I'd appreciate a design check first though)

youknowriad · 2018-10-19T16:52:29Z

cc @alexislloyd in case you have some thoughts on the design questions raised here

Components: Add info prop support to MenuItem, FeatureToggle #10802 (comment) (second point)
Components: Add info prop support to MenuItem, FeatureToggle #10802 (comment)

Avoids offset indent on wrapped text

alexislloyd · 2018-10-19T18:25:09Z

We already have two other variations of a descriptive subtext, one in the Post Visibility menu, the other of sidebar Controls (e.g. Drop Cap description). As implemented here, I've inherited precisely the style used for the sidebar controls (as proposed at #10340 (comment) and seconded at #10340 (comment)).

I think that this is an issue to resolve going forward to make sure we have consistent patterns for different type styles and usage guidelines, so that we avoid a lot of inconsistency. That's a longer-term solution though.

For a two-line list item like this, I would propose that the info prop be 2px smaller in size and slightly lighter grey (while maintaining sufficient contrast).

youknowriad · 2018-10-19T18:38:13Z

Updated the PR:

I made the menu links a bit darker, the info lighter and decreased the font-size for the info by 1px (2px felt too small)

Thoughts?

…ress#10802) * Components: Allow aria-label override on IconButton * Components: Add `info` prop support to MenuItem * Edit Post: Add `info` prop support to FeatureToggle * Edit Post: Add feature toggle info texts * Components: Menu Item: Break text beyond mobile toolbar * Components: Icon Button: Indent text by SVG margin Avoids offset indent on wrapped text * Tweak menu item styling * Avoid italic text

aduth added [Status] In Progress Tracking issues with work in progress Needs Design Feedback Needs general design feedback. Needs Accessibility Feedback Need input from accessibility labels Oct 19, 2018

aduth added this to the 4.1 - UI freeze milestone Oct 19, 2018

aduth added [Feature] UI Components Impacts or related to the UI component system and removed [Status] In Progress Tracking issues with work in progress labels Oct 19, 2018

aduth force-pushed the add/10340-menu-item-info branch from f5b14e0 to 2b50700 Compare October 19, 2018 15:49

aduth commented Oct 19, 2018

View reviewed changes

aduth mentioned this pull request Oct 19, 2018

Add Option for Descriptions within Menu Items #10340

Closed

aduth commented Oct 19, 2018

View reviewed changes

youknowriad reviewed Oct 19, 2018

View reviewed changes

youknowriad approved these changes Oct 19, 2018

View reviewed changes

aduth added 6 commits October 19, 2018 13:34

Components: Allow aria-label override on IconButton

baff503

Components: Add info prop support to MenuItem

76048f6

Edit Post: Add info prop support to FeatureToggle

21ebfe5

Edit Post: Add feature toggle info texts

76eee7d

Components: Menu Item: Break text beyond mobile toolbar

6ad1605

Components: Icon Button: Indent text by SVG margin

edf05c8

Avoids offset indent on wrapped text

aduth force-pushed the add/10340-menu-item-info branch from 889d7ff to edf05c8 Compare October 19, 2018 17:40

Tweak menu item styling

0b59b7f

Avoid italic text

eafc492

youknowriad removed the Needs Design Feedback Needs general design feedback. label Oct 19, 2018

youknowriad merged commit e8aaada into master Oct 19, 2018

youknowriad deleted the add/10340-menu-item-info branch October 19, 2018 19:08

monzerfoda mentioned this pull request Nov 28, 2023

[Snyk] Fix for 10 vulnerabilities monzerfoda/gutenberg#18

Open

afercia mentioned this pull request Feb 6, 2024

MenuItem: split content and info prop into accessible name and accessible description #58720

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Components: Add `info` prop support to MenuItem, FeatureToggle #10802

Components: Add `info` prop support to MenuItem, FeatureToggle #10802

aduth commented Oct 19, 2018 •

edited

Loading

aduth Oct 19, 2018

mtias Oct 19, 2018

aduth Oct 19, 2018

alexislloyd Oct 19, 2018

alexislloyd Oct 19, 2018

aduth Oct 19, 2018 •

edited

Loading

aduth Oct 19, 2018

youknowriad Oct 19, 2018

aduth Oct 19, 2018

youknowriad Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

aduth commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad left a comment

youknowriad commented Oct 19, 2018

alexislloyd commented Oct 19, 2018

youknowriad commented Oct 19, 2018


		// Don't wrap text until viewport is beyond the mobile breakpoint.

Components: Add info prop support to MenuItem, FeatureToggle #10802

Components: Add info prop support to MenuItem, FeatureToggle #10802

Conversation

aduth commented Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth Oct 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

aduth commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad commented Oct 19, 2018

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad commented Oct 19, 2018

alexislloyd commented Oct 19, 2018

youknowriad commented Oct 19, 2018

Components: Add `info` prop support to MenuItem, FeatureToggle #10802

Components: Add `info` prop support to MenuItem, FeatureToggle #10802

aduth commented Oct 19, 2018 •

edited

Loading

aduth Oct 19, 2018 •

edited

Loading