UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu by aduth · Pull Request #14843 · WordPress/gutenberg

aduth · 2019-04-05T16:01:06Z

Previously: #13410 (comment) , #2989
Related: #14754
Supersedes: #13059
Fixes #12505, fixes #15318 (Single visible menu is made up of multiple menus)

This pull request seeks to refactor the block settings menu to use the DropdownMenu component. This is part of a larger effort to consolidate and normalize behaviors between dropdown menus. Prior to this pull request, there was a fair amount of duplication and divergent behaviors between the block settings menu and the standard DropdownMenu component.

There is not expected to be a difference in the look and feel of the block settings menu.

Design Notes:

The changes here err on the side of treating the block settings menu styles as the more correct iteration of dropdown menu styling. Notably, this has the following impact:

The right margin of an icon in DropdownMenu has increased by 1px, from 4px to 5px
Default left- and right-padding of the DropdownMenu contents has been removed. This affects existing usage in the table block Edit Table dropdown (before, after).
Small viewport hover styling has been restored (previously removed by Hover fix #10333 in an attempt to remove tap-drag hover activation described in (Mobile) :hover styles on component menus are following thumb movements #10320). Again noting that this never applied previously for block settings menu, but did apply for "More Options" and "Edit Table" dropdowns. This may need design confirmation. However, I would argue that it is not safe to assume that the relationship between touch device and small viewports are mutually inclusive (e.g. laptops with touchscreen support).

Implementation Notes:

I operated on an assumption that the fact that block settings menus and "More Options" menu were not implemented as DropdownMenu was at least partly motivated by the amount of additional customization necessary for these menus being unsupportable by the base component. It's for this reason that I introduced a new children render function prop to serve as an alternative to the existing (limiting) controls prop.

As an alternative, I had also explored whether to allow elements to be intermixed with control object definitions in the default controls prop. There is a possible route here, but it is made difficult by the fact that (a) the menu items are likely to need access to onClose, (b) each element in an array needing a key, and (c) the confusion surrounding an overloaded array supports of an object and element formats. It's for this reason that I decided to stick with the render function and, if anything, I would suggest that in retrospect this makes more sense as a default anyways.

It may help to review using GitHub's "Ignore white space changes" option (especially BlockSettingsMenu).

Testing Instructions:

Verify that the block settings menu (right-most in toolbar) looks and behaves effectively identical as to master.

Verify that More Menu (right-most in header) looks and behaves identically as to master with one exception:

using up/down arrow keys inside the menu allows you to navigate between all items, it doesn't lock you down inside a single group as proposed in The top bar More Menu should be a unique ARIA menu #12505

Ensure unit tests pass:

npm run test-unit

Included Tasks:

Refactor "More Options" as DropdownMenu
Refactor MenuGroup to stop using NavigableMenu which wrongly narrows down arrow keys navigation

Known bugs:

There are 2 bugs which are being tackled separately:

Focus wrongly transitions to post title when toggling Visual/Code (Edit Post: Focus wrongly transitions to post title when toggling Visual/Code #14850)
More Options menu toggle buttons cause focus loss when toggled (Edit Post: More Options menu toggle buttons cause focus loss when toggled #14849)

Future Tasks:

Refactor DropdownMenu to use MenuItem component in its default rendering of controls

mapk · 2019-04-05T22:04:47Z

Verify that the block settings menu (right-most in toolbar) looks and behaves effectively identical as to master.

Here's a comparison of what I see in Firefox between this feature branch and master.

The ellipses (3 dots) is now vertically aligned.
Top padding in the popover is increased before the first menu item.
There a horizontal rule separating out the "Remove block" option.
Bottom padding in the popover is increased after the last menu item.

jasmussen · 2019-04-08T07:05:09Z

Really nice work. Thank you for doing this.

Verify that the block settings menu (right-most in toolbar) looks and behaves effectively identical as to master.

This is what I see in both this branch and in master:

In other words, I don't see the same "old" style as @mapk does in master, not sure why. But in any case as far as my testing goes, this GIF is correct looking, and very nice.

I did notice one thing. Master:

This branch:

Since that menu is not part of the scope of this branch, feel free to ignore it. But as you can see that overflow menu is actually slightly improved in this branch, all that's missing is the correct hover style. If fixing that hover style is within reach, it'd be nice to fix!

Otherwise, feel free to ship once we work out why Mark saw something different than I did.

gziolo

Noting, that there is #12505 opened which describes anothe issue related to more menus - the top bar More Menu should be a unique ARIA menu. There is #13059 opened which tries to solve it. I raised an issue when doing review which reads as follows:

It seems like we need to introduce another component called Menu which will have role set to menu, its class name which will hide the label for the menu, and will always have useEventToOffset set to true. This will make it possible to leave MenuGroup usage unchanged.

My point here is, that we should ensure that we refactor both menus in a way where they use the same symantics.

gziolo · 2019-04-08T08:46:25Z

+								<__experimentalBlockSettingsMenuPluginsExtension.Slot
+									fillProps={ { clientIds, onClose } }
+								/>
+								<DropdownMenuSeparator />


I have some concerns about this component which is 100% visual and won't be noticeable for those having sight issues. The question is whether this separator exists only for better aesthetics or should menu groups be used instead?

I have some concerns about this component which is 100% visual and won't be noticeable for those having sight issues. The question is whether this separator exists only for better aesthetics or should menu groups be used instead?

It's a very good point. Between your comment and #12505, it seems clear that proper groupings would have preferable semantics than what can be communicated with this line. I could see an attempt at an argument that some lines are truly intended to be purely visual flourishes, though it seems a weak one and certainly one which should be limited, if not actively discouraged. At the very least, it's not clear to me why not to use a proper hr element over the existing div.

We have the MenuGroup component which might serve some help here, though it seems to assume a label not currently present in the block settings menu.

The more I look at the menu entries and trying to consider what these groupings might be, the more I wonder if Remove Block really is in any way of a different classification than the other items, or if the line truly does just serve as visually emphasis (for its frequency? as an indicator of danger?).

Action items:

Don't expose this as a publicly-available component

Decide whether MenuGroup is possible, recommended (advice welcome)

aduth · 2019-04-08T13:02:21Z

Thanks @mapk @jasmussen for spotting those styling issues! It's quite possible I'd overlooked a few, so I'll plan to take another pass at it. I'll note that it's proving to be a useful exercise in consistency, as those "inaccuracies" are exactly what one would expect the behavior for their own default DropdownMenu usage prior to this pull request.

Since that menu is not part of the scope of this branch, feel free to ignore it. But as you can see that overflow menu is actually slightly improved in this branch, all that's missing is the correct hover style. If fixing that hover style is within reach, it'd be nice to fix!

I must have missed that that this menu was using DropdownMenu. I was largely operating under the assumption that only the "Edit Table" dropdown was using it. I think it's fair to say that the scope of this pull request would be to align the behaviors of all DropdownMenu toward what's implemented today as the block settings menu. I'll work to fix it up!

gziolo · 2019-04-12T13:43:15Z

I applied the following changes to this PR:

rebased this branch with master first
refactored MoreMenu component to use DropdownMenu
refactored DropdownMenu to use all props that MoreMenu was using
refactored MenuGroup to stop using NavigableMenu and added the same attributes as proposed in The top bar More Menu should be a unique ARIA menu #12505

I left more comments inline. DropdownMenu still needs to be verified whether the current proposal offers solid public API after changes applied. Once it is sorted out, docs need to be updated with new props added and CHANGELOG file as well.

Co-Authored-By: gziolo <grzegorz@gziolo.pl>

…nstable

gziolo · 2019-05-30T14:35:17Z

I'm afk today, but just merged a tweak to some menus. Try master again, and maybe rebase.

Rebased, I think I did it 2 hours ago already :) I think that the current proposal is very consistent and I'm personally happy with it.

I also addressed the last remaining comment from @aduth (#14843 (comment)) with 032167a.

gziolo

This is looking to be in good shape. Left a few comments.

I can't approve my own pull request, so you'll need to do as your discretion once the points are considered.

I'm approving this PR based on the feedback received so far. I will wait with merge until Monday to get other folks a chance to chime in. Thank you, everyone, for all the help so far ❤️

youknowriad · 2019-05-31T10:03:29Z

Tested this quickly and it seems to work well.

gziolo · 2019-06-03T07:35:06Z

Let's get this in. I will open a follow-up issue to get rid of all __unstable props and see if there is a quick way to solve it.

gziolo · 2019-06-03T07:47:25Z

Let's get this in. I will open a follow-up issue to get rid of all __unstable props and see if there is a quick way to solve it.

I opened #15960 to address it. @aduth - do we have issues opened for observed focus loss in More Menu?

afercia · 2019-06-03T08:15:01Z

Thanks everyone, this is a big improvement for the menu ❤️

@gziolo re: the focus loss, I've created #15501 a while ago.

gziolo · 2019-06-03T15:17:59Z

Let's get this in. I will open a follow-up issue to get rid of all __unstable props and see if there is a quick way to solve it.

I opened #15960 to address it.

PR is ready: #15968.

aduth added Needs Design Feedback Needs general design feedback. [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Package] Block editor /packages/block-editor labels Apr 5, 2019

aduth requested a review from jasmussen April 5, 2019 16:01

aduth requested review from ajitbohra, chrisvanpatten, ellatrix, gziolo, jorgefilipecosta, noisysocks, talldan and youknowriad as code owners April 5, 2019 16:01

aduth commented Apr 5, 2019

View reviewed changes

Comment thread packages/block-editor/src/components/block-settings-menu/index.js

aduth commented Apr 5, 2019

View reviewed changes

Comment thread packages/block-editor/src/components/block-settings-menu/index.js

aduth commented Apr 5, 2019

View reviewed changes

Comment thread packages/block-editor/src/components/block-settings-menu/style.scss Outdated

aduth added the Needs Accessibility Feedback Need input from accessibility label Apr 5, 2019

gziolo reviewed Apr 8, 2019

View reviewed changes

aduth mentioned this pull request Apr 8, 2019

Fix issue with top bar menu having unique ARIA roles #13059

Closed

5 tasks

gziolo mentioned this pull request Apr 10, 2019

The top bar More Menu should be a unique ARIA menu #12505

Closed

gziolo assigned aduth and gziolo Apr 10, 2019

gziolo force-pushed the update/block-settings-dropdown-menu branch from 38efd4f to bfdd43e Compare April 12, 2019 12:00

gziolo reviewed Apr 12, 2019

View reviewed changes

Comment thread packages/block-editor/src/components/block-settings-menu/index.js

Comment thread packages/components/src/dropdown-menu/index.js Outdated

Comment thread packages/components/src/dropdown-menu/index.js Outdated

Comment thread packages/components/src/dropdown-menu/index.js Outdated

gziolo changed the title ~~Block Editor: Refactor BlockSettingsMenu as DropdownMenu~~ Block Editor: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu Apr 12, 2019

gziolo changed the title ~~Block Editor: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu~~ UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu Apr 12, 2019

gziolo and others added 11 commits May 30, 2019 16:18

Refactor MoreMenu component to use DropdownMenu component

51acab9

Use single label for DrowdownMenu components

2d34643

Fix all failing e2e tests

cf462a6

Add all class names for backward compatibility

7ea821b

Fix unit tests by refreshing saved snapshots

dc93e96

Update packages/components/src/menu-group/index.js

4864809

Co-Authored-By: gziolo <grzegorz@gziolo.pl>

Add back removed changelog update for components during rebase

67a4f8b

Mark all props used with DropdownMenu for backward compatibility as u…

6276b9c

…nstable

Remove obsolete DropdownMenuSepararator component

2456dde

Apply changes after rebase to align CSS styles with master

1b24803

Display dropdown menu indicator only when icon set expliclity to false

032167a

gziolo force-pushed the update/block-settings-dropdown-menu branch from afa7bfa to 032167a Compare May 30, 2019 14:34

gziolo approved these changes May 31, 2019

View reviewed changes

gziolo added this to the 5.9 (Gutenberg) milestone May 31, 2019

gziolo merged commit c2768ec into master Jun 3, 2019

gziolo deleted the update/block-settings-dropdown-menu branch June 3, 2019 07:35

gziolo removed the Needs Design Feedback Needs general design feedback. label Jun 3, 2019

gziolo mentioned this pull request Jun 3, 2019

Components: Stop using unstable props in DropdownMenu #15960

Closed

afercia mentioned this pull request Jun 3, 2019

Find a better place for the "Manage All Reusable Blocks" link #13390

Closed

gziolo mentioned this pull request Jun 3, 2019

Components: Refactor DropdownMenu to stop using unstable props #15968

Merged

gziolo mentioned this pull request Aug 2, 2019

DropdownMenu: Fix shifting menu items #16871

Merged

swissspidy mentioned this pull request Aug 5, 2019

E2E Tests: Block Manager disabled ampproject/amp-wp#2904

Closed

Uh oh!

Conversation

aduth commented Apr 5, 2019 • edited by gziolo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mapk commented Apr 5, 2019

Uh oh!

jasmussen commented Apr 8, 2019

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo Apr 8, 2019

Choose a reason for hiding this comment

Uh oh!

aduth Apr 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Apr 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gziolo commented Apr 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

youknowriad commented May 31, 2019

Uh oh!

gziolo commented Jun 3, 2019

Uh oh!

gziolo commented Jun 3, 2019

Uh oh!

afercia commented Jun 3, 2019

Uh oh!

gziolo commented Jun 3, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduth commented Apr 5, 2019 •

edited by gziolo

Loading

aduth Apr 8, 2019 •

edited

Loading

gziolo commented Apr 12, 2019 •

edited

Loading

gziolo commented May 30, 2019 •

edited

Loading