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

Blocks: Introduce block settings menu toggle #2884

Merged
merged 7 commits into from Oct 9, 2017

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Oct 5, 2017

Replaces the menu at the right of any block by an ellipsis menu.

Screenshots

screen shot 2017-10-05 at 09 37 03

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Oct 5, 2017

Codecov Report

Merging #2884 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2884      +/-   ##
==========================================
- Coverage   34.06%   33.82%   -0.24%     
==========================================
  Files         192      193       +1     
  Lines        5677     5729      +52     
  Branches      997     1008      +11     
==========================================
+ Hits         1934     1938       +4     
- Misses       3166     3203      +37     
- Partials      577      588      +11
Impacted Files Coverage Δ
editor/actions.js 38.88% <0%> (-2.29%) ⬇️
editor/sidebar/header.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/content.js 0% <0%> (ø)
editor/header/index.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
blocks/library/gallery/block.js 9.52% <0%> (-1.59%) ⬇️
blocks/library/audio/index.js 14.54% <0%> (+1.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a02528...8977595. Read the comment docs.

codecov bot commented Oct 5, 2017

Codecov Report

Merging #2884 into master will decrease coverage by 0.23%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2884      +/-   ##
==========================================
- Coverage   34.06%   33.82%   -0.24%     
==========================================
  Files         192      193       +1     
  Lines        5677     5729      +52     
  Branches      997     1008      +11     
==========================================
+ Hits         1934     1938       +4     
- Misses       3166     3203      +37     
- Partials      577      588      +11
Impacted Files Coverage Δ
editor/actions.js 38.88% <0%> (-2.29%) ⬇️
editor/sidebar/header.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/content.js 0% <0%> (ø)
editor/header/index.js 0% <0%> (ø) ⬆️
editor/block-settings-menu/index.js 0% <0%> (ø) ⬆️
blocks/library/gallery/block.js 9.52% <0%> (-1.59%) ⬇️
blocks/library/audio/index.js 14.54% <0%> (+1.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a02528...8977595. Read the comment docs.

@youknowriad youknowriad requested review from jasmussen and mtias Oct 5, 2017

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 5, 2017

Contributor

Visually I love this, I think it's worth trying.

Accessibility wise, it seems like there are sometimes two tab stops between items in the toolbar for me. This is probably an issue separate to the ellipsis menu, but figured I'd mention it.

I pushed a fix to the placement of a few things.

One thing — can we do it so clicking the ellipsis menu also selects the block? Right now if you only hover over the block, click the ellipsis menu, and move the cursor away from the block, the ellipsis menu collapses, making it fiddly to use.

Contributor

jasmussen commented Oct 5, 2017

Visually I love this, I think it's worth trying.

Accessibility wise, it seems like there are sometimes two tab stops between items in the toolbar for me. This is probably an issue separate to the ellipsis menu, but figured I'd mention it.

I pushed a fix to the placement of a few things.

One thing — can we do it so clicking the ellipsis menu also selects the block? Right now if you only hover over the block, click the ellipsis menu, and move the cursor away from the block, the ellipsis menu collapses, making it fiddly to use.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 5, 2017

Contributor

can we do it so clicking the ellipsis menu also selects the block?

Great idea 👍

Contributor

youknowriad commented Oct 5, 2017

can we do it so clicking the ellipsis menu also selects the block?

Great idea 👍

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Oct 5, 2017

Contributor

Given the select fix, 👍 👍 from me, worth testing this.

Contributor

jasmussen commented Oct 5, 2017

Given the select fix, 👍 👍 from me, worth testing this.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Oct 5, 2017

Member

Accessibility wise, it seems like there are sometimes two tab stops between items in the toolbar for me. This is probably an issue separate to the ellipsis menu, but figured I'd mention it.

I've noticed this as well. Thought it might have been something with #2323 shifting focus into popover (tooltips are implemented as popovers), but this is meant to have been fixed with #2771 (including for Tooltip, not just AutoComplete).

Member

aduth commented Oct 5, 2017

Accessibility wise, it seems like there are sometimes two tab stops between items in the toolbar for me. This is probably an issue separate to the ellipsis menu, but figured I'd mention it.

I've noticed this as well. Thought it might have been something with #2323 shifting focus into popover (tooltips are implemented as popovers), but this is meant to have been fixed with #2771 (including for Tooltip, not just AutoComplete).

@gziolo

I still need to test it, but it looks very good. I had only nitpicks, which aren't blockers/

*
* @return {Object} Action object
*/
export function toggleSidebar() {

This comment has been minimized.

@gziolo

gziolo Oct 9, 2017

Member

I noticed that we have unit tests for a few existing action creators, should we cover those 2, too?

@gziolo

gziolo Oct 9, 2017

Member

I noticed that we have unit tests for a few existing action creators, should we cover those 2, too?

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2017

Contributor

Yeah! I know and I've written some of these, but honestly, I'm not sure these add any value for action creators just returning an action with the arguments as properties of the object.

@youknowriad

youknowriad Oct 9, 2017

Contributor

Yeah! I know and I've written some of these, but honestly, I'm not sure these add any value for action creators just returning an action with the arguments as properties of the object.

This comment has been minimized.

@gziolo

gziolo Oct 9, 2017

Member

Yes, sometimes it's hard to justify the need for writing tests, but at the same time it's very easy to write them. It's your call, as I mentioned already. It's not a blocker.

@gziolo

gziolo Oct 9, 2017

Member

Yes, sometimes it's hard to justify the need for writing tests, but at the same time it's very easy to write them. It's your call, as I mentioned already. It's not a blocker.

Show outdated Hide outdated editor/block-settings-menu/style.scss
undo: () => dispatch( { type: 'UNDO' } ),
redo: () => dispatch( { type: 'REDO' } ),
toggleSidebar: () => dispatch( { type: 'TOGGLE_SIDEBAR' } ),
onToggleSidebar: () => dispatch( toggleSidebar() ),

This comment has been minimized.

@gziolo

gziolo Oct 9, 2017

Member

New action creator 👍

@gziolo

gziolo Oct 9, 2017

Member

New action creator 👍

@@ -35,7 +49,7 @@
display: block;
}
&:first-child {
margin-bottom: 4px;
.editor-block-settings-menu__content &:first-child {

This comment has been minimized.

@gziolo

gziolo Oct 9, 2017

Member

It looks like this can be moved to line 17. We have rules for .editor-block-settings-menu__content there.

@gziolo

gziolo Oct 9, 2017

Member

It looks like this can be moved to line 17. We have rules for .editor-block-settings-menu__content there.

This comment has been minimized.

@youknowriad

youknowriad Oct 9, 2017

Contributor

Don't you think it makes more sense to keep it in .editor-block-settings-menu__control because it's applied to this class when it's first on the list? I can see arguments for both which makes the choice difficult.

@youknowriad

youknowriad Oct 9, 2017

Contributor

Don't you think it makes more sense to keep it in .editor-block-settings-menu__control because it's applied to this class when it's first on the list? I can see arguments for both which makes the choice difficult.

This comment has been minimized.

@gziolo

gziolo Oct 9, 2017

Member

It takes some time to process those class names either way. CSS is complicated. We could add another class name, but having a section .editor-block-settings-menu__inspector just to change the margin is most likely be too much here.

@gziolo

gziolo Oct 9, 2017

Member

It takes some time to process those class names either way. CSS is complicated. We could add another class name, but having a section .editor-block-settings-menu__inspector just to change the margin is most likely be too much here.

@gziolo

This comment has been minimized.

Show comment
Hide comment
@gziolo

gziolo Oct 9, 2017

Member

I found only one small thing that might require a fix:

screen shot 2017-10-09 at 14 15 47

I think it should be displayed Close Setting Menu when the menu is in the opened state.

Member

gziolo commented Oct 9, 2017

I found only one small thing that might require a fix:

screen shot 2017-10-09 at 14 15 47

I think it should be displayed Close Setting Menu when the menu is in the opened state.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Oct 9, 2017

Contributor

Good catch @gziolo should be better now

Contributor

youknowriad commented Oct 9, 2017

Good catch @gziolo should be better now

@gziolo

gziolo approved these changes Oct 9, 2017

Everything looking great with the last commit. Feel free to merge once Travis will turn green 👍

@gziolo gziolo changed the title from Try/block settings menu toggle to Blocks: Introduce block settings menu toggle Oct 9, 2017

@youknowriad youknowriad merged commit 8557b53 into master Oct 9, 2017

3 checks passed

codecov/project 33.82% (-0.24%) compared to 0a02528
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad referenced this pull request Oct 9, 2017

Merged

Editor: HTML Editor per block #2797

5 of 5 tasks complete

@aduth aduth deleted the try/block-settings-menu-toggle branch Oct 9, 2017

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