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 block settings menu to the block toolbar #9572

Merged
merged 13 commits into from Sep 5, 2018

Conversation

@youknowriad
Contributor

youknowriad commented Sep 3, 2018

Extracted from #9282 and related to #9074 #9075

In an effort to split and streamline the efforts to refactor the toolbar, this PR moves the block settings menu to the block's toolbar.

Follow-up PRs will implement collapsing toolbars for the block toolbar.

Testing instructions

  • Check the block settings menu is showing up properly in all circumstances.

@youknowriad youknowriad self-assigned this Sep 3, 2018

@youknowriad youknowriad requested review from mtias, jasmussen and WordPress/gutenberg-core Sep 3, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 3, 2018

Contributor

Looking good, but it seems it's missing from multi-selection.

Contributor

mtias commented Sep 3, 2018

Looking good, but it seems it's missing from multi-selection.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 3, 2018

Contributor

Good catch @mtias it should be fixed now.

Contributor

youknowriad commented Sep 3, 2018

Good catch @mtias it should be fixed now.

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 3, 2018

Contributor

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

Contributor

mtias commented Sep 3, 2018

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

@jorgefilipecosta

The code changes are looking good 👍 From the functionality point of view I did a set of tests and I think may have found a bug.
I think we are not showing the ellipsis menu for invalid blocks.
Before:
sep-03-2018 20-35-02
After:
sep-03-2018 20-35-06

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 4, 2018

Contributor

I am seeing some styling issues in Firefox which do not occur in Chromium:

image

EDIT: It seems to have something to do with some CSS setting line-height to 1.8.

Contributor

ZebulanStanphill commented Sep 4, 2018

I am seeing some styling issues in Firefox which do not occur in Chromium:

image

EDIT: It seems to have something to do with some CSS setting line-height to 1.8.

* This seemingly redundant wrapper node avoids root return
* element styling impacting popover positioning.
*/ }
<div>

This comment has been minimized.

@youknowriad

youknowriad Sep 4, 2018

Contributor

In my testing, this div was not necessary anymore. Can you confirm @aduth?

I removed it from here and from the slots because it messes with the styling of the block toolbars.

@youknowriad

youknowriad Sep 4, 2018

Contributor

In my testing, this div was not necessary anymore. Can you confirm @aduth?

I removed it from here and from the slots because it messes with the styling of the block toolbars.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

@ZebulanStanphill I fixed the styling issue

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

@mtias I'm not certain where this behavior was triggered? Do you know where it was introduced?

Contributor

youknowriad commented Sep 4, 2018

@ZebulanStanphill I fixed the styling issue

Great. One more thing is that before we were focusing the ellipsis menu after a multi-selection. We should probably focus on the toolbar now.

@mtias I'm not certain where this behavior was triggered? Do you know where it was introduced?

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 4, 2018

Contributor

This is too cool.

Not critical, but can we control the default direction the popout opens in?

Right now it seems to default to opening down and left:

screen shot 2018-09-04 at 11 17 33

It would be nice if it could open down and right instead — until there's no space to the right of course.

Contributor

jasmussen commented Sep 4, 2018

This is too cool.

Not critical, but can we control the default direction the popout opens in?

Right now it seems to default to opening down and left:

screen shot 2018-09-04 at 11 17 33

It would be nice if it could open down and right instead — until there's no space to the right of course.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

Ok I restored the "Focus toolbar on Multiselection" but I'm focusing the first tabbable in the toolbar instead of the toolbar container for now. I can make the toolbar container a tabbable element but I wasn't sure about the a11y impacts.

I also updated the dropdown to open to the right by default.

Contributor

youknowriad commented Sep 4, 2018

Ok I restored the "Focus toolbar on Multiselection" but I'm focusing the first tabbable in the toolbar instead of the toolbar container for now. I can make the toolbar container a tabbable element but I wasn't sure about the a11y impacts.

I also updated the dropdown to open to the right by default.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

Contributor

youknowriad commented Sep 4, 2018

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

@mtias

mtias approved these changes Sep 4, 2018

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 4, 2018

Contributor

I am noticing a styling issue in the block inspector:

image

Not sure if it is related or if it was introduced by some other PR, though.

Contributor

ZebulanStanphill commented Sep 4, 2018

I am noticing a styling issue in the block inspector:

image

Not sure if it is related or if it was introduced by some other PR, though.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

Also, the e2e tests caught another issue here where the block keyboard shortcuts were not working because the block settings menu is not always rendered.

To fix it, I refactored how those keywords work consolidating all the keyboard shortcuts in the EditorGlobalKeyboardShortcuts (I think this component needs a small refactoring but I left it for another PR).

By doing so and to avoid duplication, I created a new component BlockActions using render props that take clientIds as a prop and provide onDuplicate, onRemove... for these ids. This component is used both for the global editor shortcuts and for the block settings menu.

I think it's an improvement over the current code but would be great if @talldan can do a sanity check as I believe you worked on the initial implementation.

Contributor

youknowriad commented Sep 4, 2018

Also, the e2e tests caught another issue here where the block keyboard shortcuts were not working because the block settings menu is not always rendered.

To fix it, I refactored how those keywords work consolidating all the keyboard shortcuts in the EditorGlobalKeyboardShortcuts (I think this component needs a small refactoring but I left it for another PR).

By doing so and to avoid duplication, I created a new component BlockActions using render props that take clientIds as a prop and provide onDuplicate, onRemove... for these ids. This component is used both for the global editor shortcuts and for the block settings menu.

I think it's an improvement over the current code but would be great if @talldan can do a sanity check as I believe you worked on the initial implementation.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2018

Contributor

@ZebulanStanphill Good catch, it's fixed.

Contributor

youknowriad commented Sep 4, 2018

@ZebulanStanphill Good catch, it's fixed.

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 4, 2018

Contributor

Summary of failing tests:

Summary of all failing tests
FAIL test/e2e/specs/links.test.js (18.239s)
  ● Links › can be edited
    No node found for selector: button[aria-label="Edit"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
  ● Links › can be removed
    No node found for selector: button[aria-label="Unlink"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
Test Suites: 1 failed, 29 passed, 30 total
Tests:       2 failed, 3 skipped, 105 passed, 110 total
Snapshots:   48 passed, 48 total
Time:        206.473s
Ran all test suites.

https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/links.test.js

Contributor

ZebulanStanphill commented Sep 4, 2018

Summary of failing tests:

Summary of all failing tests
FAIL test/e2e/specs/links.test.js (18.239s)
  ● Links › can be edited
    No node found for selector: button[aria-label="Edit"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
  ● Links › can be removed
    No node found for selector: button[aria-label="Unlink"]
      at assert (node_modules/puppeteer/lib/helper.js:282:11)
      at Frame.click (node_modules/puppeteer/lib/FrameManager.js:590:5)
Test Suites: 1 failed, 29 passed, 30 total
Tests:       2 failed, 3 skipped, 105 passed, 110 total
Snapshots:   48 passed, 48 total
Time:        206.473s
Ran all test suites.

https://github.com/WordPress/gutenberg/blob/master/test/e2e/specs/links.test.js

@jorgefilipecosta

This comment has been minimized.

Show comment
Hide comment
@jorgefilipecosta

jorgefilipecosta Sep 4, 2018

Member

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

The most useful features of the settings menu for invalid blocks is the ability to remove the block and to go to the HTML edit mode so we can apply a simple fix to the block. But I guess that if we have another settings menu for this blocks maybe this actions should be available there and it is ok to remove the other settings menu from the toolbar. We can add this actions there after the merge of this PR.

Member

jorgefilipecosta commented Sep 4, 2018

@jorgefilipecosta I wonder if it's better to keep this way. It suggests the user has to make a decision before going forward with the block.

The most useful features of the settings menu for invalid blocks is the ability to remove the block and to go to the HTML edit mode so we can apply a simple fix to the block. But I guess that if we have another settings menu for this blocks maybe this actions should be available there and it is ok to remove the other settings menu from the toolbar. We can add this actions there after the merge of this PR.

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 4, 2018

Member

This could be my install but running this I do see some funky stuff:

2018-09-04 at 19 11

2018-09-04 at 19 11

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

Member

karmatosed commented Sep 4, 2018

This could be my install but running this I do see some funky stuff:

2018-09-04 at 19 11

2018-09-04 at 19 11

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 4, 2018

Member

This could be my install but running this I do see some funky stuff:

Common pitfall with falsy (not false) conditions:

https://codepen.io/aduth/pen/dWwzrL

Member

aduth commented Sep 4, 2018

This could be my install but running this I do see some funky stuff:

Common pitfall with falsy (not false) conditions:

https://codepen.io/aduth/pen/dWwzrL

@karmatosed

This comment has been minimized.

Show comment
Hide comment
@karmatosed

karmatosed Sep 4, 2018

Member

Ah, thanks for context @aduth TIL!

Member

karmatosed commented Sep 4, 2018

Ah, thanks for context @aduth TIL!

@ZebulanStanphill

This comment has been minimized.

Show comment
Hide comment
@ZebulanStanphill

ZebulanStanphill Sep 5, 2018

Contributor

@jorgefilipecosta I think that showing the Remove Block and Edit as HTML buttons in the invalid block UI (probably within its own existing ellipsis menu) is a good idea. No need to make the ellipsis visible for invalid blocks if you do that.

@karmatosed

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

Yeah, that keeps happening to me, too. I keep hovering over the right side of blocks waiting for the ellipsis to appear and then remembering that it is in the toolbar. 😛

One thing I just noticed is that there is currently no way to access the ellipsis menu for a Classic block:
image
(The extra buttons in the toolbar are from Divi; they are not relevant to this issue.)

Contributor

ZebulanStanphill commented Sep 5, 2018

@jorgefilipecosta I think that showing the Remove Block and Edit as HTML buttons in the invalid block UI (probably within its own existing ellipsis menu) is a good idea. No need to make the ellipsis visible for invalid blocks if you do that.

@karmatosed

Beyond that I had a super weird moment of cognitive breaking as I have learnt 'ellipsis to the right'. I suspect for those existing users this could be a friction, but I do think it's worth taking that hit to improve for everyone going forwards.

Yeah, that keeps happening to me, too. I keep hovering over the right side of blocks waiting for the ellipsis to appear and then remembering that it is in the toolbar. 😛

One thing I just noticed is that there is currently no way to access the ellipsis menu for a Classic block:
image
(The extra buttons in the toolbar are from Divi; they are not relevant to this issue.)

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

@jorgefilipecosta @ZebulanStanphill I restored the menu for invalid blocks. We can change later if needed.

Contributor

youknowriad commented Sep 5, 2018

@jorgefilipecosta @ZebulanStanphill I restored the menu for invalid blocks. We can change later if needed.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

@karmatosed The error should be fixed. Thanks for catching it.

Contributor

youknowriad commented Sep 5, 2018

@karmatosed The error should be fixed. Thanks for catching it.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

I restored the button for freeform block. It might not be the best design-wise but I think it's important to have it and at least it's consistent with the other blocks design wise.

Contributor

youknowriad commented Sep 5, 2018

I restored the button for freeform block. It might not be the best design-wise but I think it's important to have it and at least it's consistent with the other blocks design wise.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 5, 2018

Contributor

should work

I pushed a fix for the ellipsis button on the classic toolbar. It's not perfect, but it's a great deal better. Because if it sits above the classic blocks toolbar, then it will overlap the toolbar when the sticky positioning kicks in.

Contributor

jasmussen commented Sep 5, 2018

should work

I pushed a fix for the ellipsis button on the classic toolbar. It's not perfect, but it's a great deal better. Because if it sits above the classic blocks toolbar, then it will overlap the toolbar when the sticky positioning kicks in.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 5, 2018

Contributor

I will make a note to revisit the classic block separately to polish the responsiveness a bit. But I don't think that should stop this change.

Contributor

jasmussen commented Sep 5, 2018

I will make a note to revisit the classic block separately to polish the responsiveness a bit. But I don't think that should stop this change.

@youknowriad youknowriad merged commit 2065011 into master Sep 5, 2018

2 checks passed

codecov/project 50.92% (+0.55%) compared to ef25165
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 5, 2018

Contributor

🎉

Contributor

youknowriad commented Sep 5, 2018

🎉

@youknowriad youknowriad deleted the update/move-block-settings-menu-to-toolbar branch Sep 5, 2018

@gziolo gziolo added this to the 3.8 milestone Sep 5, 2018

@talldan

This comment has been minimized.

Show comment
Hide comment
@talldan

talldan Sep 5, 2018

Contributor

@youknowriad Sorry for the delay, looks like a good change. I had something very similar at one point during the initial development of those shortcuts. It's good they're no longer dependent on rendering the menu.

Contributor

talldan commented Sep 5, 2018

@youknowriad Sorry for the delay, looks like a good change. I had something very similar at one point during the initial development of those shortcuts. It's good they're no longer dependent on rendering the menu.

@ktmn

This comment has been minimized.

Show comment
Hide comment
@ktmn

ktmn Sep 6, 2018

this PR moves the block settings menu to the block's toolbar.

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

Small wishlist thing would be to have the Remove Block option right next to the block settings button for easy access. Are there any filters to facilitate doing that?

Small regression: you now have to focus the block to access the settings menu, previously all you needed to do was hover the right side of the block. Adds an extra click.

ktmn commented Sep 6, 2018

this PR moves the block settings menu to the block's toolbar.

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

Small wishlist thing would be to have the Remove Block option right next to the block settings button for easy access. Are there any filters to facilitate doing that?

Small regression: you now have to focus the block to access the settings menu, previously all you needed to do was hover the right side of the block. Adds an extra click.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 6, 2018

Contributor

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

I feel you on this, but I would ask you give this a few weeks to get a feel for it.

The reason is that the ellipsis is nearly always used as an "overflow" menu that sits at the end of a list of items. Given left to right reading direction, that means it sits at the end of the block toolbar, which also means in situations where there's not a lot there, like the separator which has a switcher and nothing more, it will be far to the left. But it's still the logically right place for it, and after using it for a bit I felt it was the better place to put it.

If we were to put it on the right side, we'd have to either slit the toolbar causing a more complex silhuette, or extend the toolbar with a lot of whitespace in between formatting and ellipsis, which would cover additional content before the selected block.

Contributor

jasmussen commented Sep 6, 2018

I like it better than the previous button, but I don't like that the button can be positioned on the far left of the block (when it doesn't have other toolbar items), in the middle (when it has some toolbar items) or on the far right (when it has lots of toolbar items). Would it be possible to make it fixed on either end so you know intuitively where to expect the button?

I feel you on this, but I would ask you give this a few weeks to get a feel for it.

The reason is that the ellipsis is nearly always used as an "overflow" menu that sits at the end of a list of items. Given left to right reading direction, that means it sits at the end of the block toolbar, which also means in situations where there's not a lot there, like the separator which has a switcher and nothing more, it will be far to the left. But it's still the logically right place for it, and after using it for a bit I felt it was the better place to put it.

If we were to put it on the right side, we'd have to either slit the toolbar causing a more complex silhuette, or extend the toolbar with a lot of whitespace in between formatting and ellipsis, which would cover additional content before the selected block.

@mcsf

This comment has been minimized.

Show comment
Hide comment
@mcsf

mcsf Sep 11, 2018

Contributor

Reported a related issue: #9797

Contributor

mcsf commented Sep 11, 2018

Reported a related issue: #9797

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