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

Move the remove icon button to the ellipsis menu as a menu button #7543

Merged
merged 12 commits into from Jun 28, 2018

Conversation

Projects
None yet
5 participants
@talldan
Contributor

talldan commented Jun 26, 2018

Description

Fixes #7216, moves the remove icon button to the ellipsis menu as a menu button.

How has this been tested?

  • Added unit tests for 'Remove Block' menu button
  • Performed manual tests including removing single and multiple blocks

Screenshots

screen shot 2018-06-26 at 11 24 08 am

Types of changes

  • Removed old icon button version of remove button
  • Altered existing Remove menu button (which still seemed to be present from previous version) to display like other menu buttons
  • Added tests for menu button

Checklist:

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

talldan added some commits Jun 26, 2018

Update BlockSettingsMenu BlockRemoveButton appearance
Add the label 'Remove Block' to the BlockRemoveButton and change the classname
to editor-block-settings-menu__control so the the icon button is consistent
with other menu controls

@talldan talldan changed the title from Update/block remove button to Move the remove icon button to the ellipsis menu as a menu button Jun 26, 2018

@noisysocks noisysocks requested a review from karmatosed Jun 26, 2018

@noisysocks noisysocks self-requested a review Jun 26, 2018

@karmatosed

This comment has been minimized.

Member

karmatosed commented Jun 26, 2018

Thanks for working on this @talldan . Could you add a separator using the style used in other drop downs?

artboard

This visually creates a good divide in actions.

talldan added some commits Jun 27, 2018

Add MenuGroup components to separate menu items by their action
Intiially this just separates other menu items from the
'Remove block' menu item. This commit is a stepping stone to
separating both Remove Block and Delete Shared Block from the
other menu items.
Apply border between MenuGroups
Previously, this border between menu groups was only being applied to
the MenuGroups in the Edit Post More Menu. This moves it to the MenuGroup component
so that it is applied to all MenuGroups
Extract button for deleting shared block into its own component
Extract the button into a new SharedBlockDeleteButton, so that it can be
rendered separately in a menu group with the RemoveBlock button.
Extract test for SharedBlockDeleteButton from SharedBlockSettings
Refactor tests to match refactoring to components. Add a snapshot test
for SharedBlockDeleteButton
@talldan

This comment has been minimized.

Contributor

talldan commented Jun 27, 2018

Well, I thought adding a separator would be nice and easy, but there were a few intricacies:

  • In the other menu, a MenuGroup component is used to separate the menu items. The border styling for the separator was applied only to that menu's MenuGroups though. I've changed it now so that it applied to all MenuGroups. I had a search in the codebase, and the only thing that it might affect that I'm unsure how to view is something called PluginSidebarMoreMenuItem - I guess I'll need some plugins to view that?
  • I had to refactor a bit, since the menu items for 'Convert to Regular Block' and 'Delete Shared Block' were actually rendered in one parent component and because of that it was impossible to get a MenuGroup around the menu items we wanted. I've extracted the Delete Shared Block into its own component.
  • Using MenuGroups in the block settings menu introduces a bit of extra padding. It could be removed using a modifier. Let me know.

Here's a couple of screenshots showing how it looks now:
screen shot 2018-06-27 at 5 26 49 pm
screen shot 2018-06-27 at 5 27 13 pm

@talldan

This comment has been minimized.

Contributor

talldan commented Jun 27, 2018

Another very little thing I thought. It feels like when the menu is open, the ellipsis icon button should really not become invisible. In my screenshots above, the popover menu feels like its calling out from nowhere.

@mtias mtias added this to the 3.2 milestone Jun 27, 2018

@aduth aduth referenced this pull request Jun 27, 2018

Closed

chore: Move "Remove" block button to ellipsis menu #7586

4 of 4 tasks complete
@karmatosed

This comment has been minimized.

Member

karmatosed commented Jun 27, 2018

This looks great and thanks for the hardwork wrangling this @talldan. I agree let's have the ellipsis show.

@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 28, 2018

I've changed it now so that it applied to all MenuGroups. I had a search in the codebase, and the only thing that it might affect that I'm unsure how to view is something called PluginSidebarMoreMenuItem - I guess I'll need some plugins to view that?

You can test the plugins menu by pasting this snippet (the ES5 version) into your DevTools console. This will add a button into the More menu. See #6031 for some context on that feature.

@noisysocks

noisysocks requested changes Jun 28, 2018 edited

Great work on this! It's looking really really close.

I'm not sure about re-using MenuGroup in this way. I support the general idea but there's a few implications:

  1. We now have two <div role="menu">s nested within a <div role="menu"> which feels a little weird to me.

screen shot 2018-06-28 at 10 54 08

  1. The child <div role="menu">s have an aria-labelledby attribute which refers to an identifier that doesn't exist. Admittedly, this is probably a bug in MenuGroup.

  2. Moving the border-bottom rule to MenuGroup has caused some regressions with PluginSidebarMoreMenuItem, like you suspected it would:

screen shot 2018-06-28 at 10 56 16

  1. The extra padding—like you said 🙂

I notice that we have a .editor-block-settings-menu__separator class defined already, so an alternative to MenuGroup might be to simply use a div with that class to add the separator line. You can see an example of us doing exactly that in editor/components/block-settings-menu/block-transformations.js.

@@ -12,7 +12,7 @@ import { __ } from '@wordpress/i18n';
import { isSharedBlock } from '@wordpress/blocks';
import { withSelect, withDispatch } from '@wordpress/data';
export function SharedBlockSettings( { sharedBlock, onConvertToStatic, onConvertToShared, onDelete, itemsRole } ) {
export function SharedBlockSettings( { sharedBlock, onConvertToStatic, onConvertToShared, itemsRole } ) {

This comment has been minimized.

@noisysocks

noisysocks Jun 28, 2018

Member

Let's rename this component (and associated files/tests) to something like SharedBlockConvertButton since it only does one thing now.

);
const text = wrapper.find( 'IconButton' ).children().text();
expect( text ).toEqual( 'Delete Shared Block' );

This comment has been minimized.

@noisysocks

noisysocks Jun 28, 2018

Member

I don't think this assertion is useful—we're already "testing" the label text via the snapshot test above.

import { isSharedBlock } from '@wordpress/blocks';
import { withSelect, withDispatch } from '@wordpress/data';
export function SharedBlockDeleteButton( { sharedBlock, onDelete, itemsRole } ) {

This comment has been minimized.

@noisysocks

noisysocks Jun 28, 2018

Member

I like that these components only have one responsibility now 🙂🌈

talldan added some commits Jun 28, 2018

Replace menu groups with a div that has a separator class applied
Using menu groups introduces a few issues in this menu
- They have extra padding by default
- They introduce another role=menu attribute
- the aria-labelledby attribute refers to a non-existant id

The separator class is already used for block transformations in this
menu so is a good fit.
Rename SharedBlockSettings component to SharedBlockConvertButton
Since a recent refactor, this component now only renders buttons for
converting to and from shared blocks. This commit renames it to that
effect.
@noisysocks

This comment has been minimized.

Member

noisysocks commented Jun 28, 2018

Looks and works great! Thanks for working on this @talldan—and congrats on your first Gutentribution!

screen shot 2018-06-28 at 12 22 36

Another very little thing I thought. It feels like when the menu is open, the ellipsis icon button should really not become invisible. In my screenshots above, the popover menu feels like its calling out from nowhere.

Since this bug already exists, let's address it in a seperate issue/PR.

@talldan talldan merged commit 973f51d into WordPress:master Jun 28, 2018

2 checks passed

codecov/project 47.26% (+0.01%) compared to 3318cb3
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@talldan talldan deleted the talldan:update/block-remove-button branch Jun 28, 2018

@hedgefield

This comment has been minimized.

hedgefield commented Jul 3, 2018

LOVE THIS.

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