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

UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu #14843

Merged
merged 18 commits into from Jun 3, 2019

Conversation

@aduth
Copy link
Member

commented Apr 5, 2019

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 #10333 in an attempt to remove tap-drag hover activation described in #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 #12505

more-menu

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 (#14850)
  • 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

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

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.

side-by-side

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

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

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:

branch

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:

Screenshot 2019-04-08 at 09 02 49

This branch:

Screenshot 2019-04-08 at 09 02 27

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
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@gziolo

gziolo Apr 8, 2019

Member

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?

This comment has been minimized.

Copy link
@aduth

aduth Apr 8, 2019

Author Member

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

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

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!

@aduth aduth referenced this pull request Apr 8, 2019

Closed

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

5 of 5 tasks complete

@gziolo gziolo force-pushed the update/block-settings-dropdown-menu branch from 38efd4f to bfdd43e Apr 12, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

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 #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.

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

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

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

@gziolo

This comment has been minimized.

Copy link
Member

commented May 30, 2019

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

gziolo approved these changes May 31, 2019

Copy link
Member

left a comment

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 ❤️

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

@youknowriad

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

Tested this quickly and it seems to work well.

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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 gziolo merged commit c2768ec into master Jun 3, 2019

1 check passed

Travis CI - Pull Request Build Passed
Details

@gziolo gziolo deleted the update/block-settings-dropdown-menu branch Jun 3, 2019

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jun 3, 2019

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

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

@gziolo

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

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.

nicolad added a commit to nicolad/gutenberg that referenced this pull request Jun 15, 2019

UI Components: Refactor BlockSettingsMenu and MoreMenu as DropdownMenu (
WordPress#14843)

* Components: Document missing menuLabel, position, className Dropdown props

* Components: Add render prop support to DropdownMenu

* Block Editor: Refactor BlockSettingsMenu as DropdownMenu

* CHANGELOG 7a54cec

* CHANGELOG 743e4da

* CHANGELOG ea82658

* Components: Remove unintended tabs from DropdownMenu tests

* Refactor MoreMenu component to use DropdownMenu component

* Use single label for DrowdownMenu components

* Fix all failing e2e tests

* Add all class names for backward compatibility

* Fix unit tests by refreshing saved snapshots

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

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

* Add back removed changelog update for components during rebase

* Mark all props used with DropdownMenu for backward compatibility as unstable

* Remove obsolete DropdownMenuSepararator component

* Apply changes after rebase to align CSS styles with master

* Display dropdown menu indicator only when icon set expliclity to false
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.