Blocks: Introduce block settings menu toggle by youknowriad · Pull Request #2884 · WordPress/gutenberg

youknowriad · 2017-10-05T08:40:05Z

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

Screenshots

codecov · 2017-10-05T08:40:07Z

Codecov Report

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

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

jasmussen · 2017-10-05T09:01:40Z

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 · 2017-10-05T09:59:58Z

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

Great idea 👍

jasmussen · 2017-10-05T10:34:13Z

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

aduth · 2017-10-05T20:02:58Z

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

aduth · 2017-10-05T21:14:13Z

editor/block-settings-menu/index.js

+					className={ toggleClassname }
+					onClick={ this.toggleMenu }
+					icon="ellipsis"
+					aria-label={ __( 'Open Settings Menu' ) }


We should assign this prop as label, which has the same effect of applying aria-label, but also enables the tooltip to be shown when hovering the icon.

Actually, I did it intentionally to avoid the tooltip, I thought it was not necessary here, but I can change.

Hmm, I guess it's to a broader question of when and where to apply tooltips. My philosophy has been if there is no adjacent text explaining what the icon is for, that a tooltip is a good idea.

aduth · 2017-10-05T21:14:48Z

editor/block-settings-menu/content.js

+	} ),
+	( dispatch, ownProps ) => ( {
+		onDelete() {
+			dispatch( {


While we're here, can we refactor this to use the removeBlock action creator?

aduth · 2017-10-05T21:15:04Z

editor/block-settings-menu/content.js

+			dispatch( selectBlock( ownProps.uid ) );
+		},
+		setActivePanel() {
+			dispatch( {


Per above, maybe also creating a new action creator for this.

aduth · 2017-10-05T21:16:04Z

editor/block-settings-menu/content.js

+import { selectBlock } from '../actions';
+
+function BlockSettingsMenuContent( { onDelete, onSelect, isSidebarOpened, toggleSidebar, setActivePanel } ) {
+	const toggleInspector = () => {


As much as I dislike the class component, I feel it has the distinct advantage in avoiding reconciliation which will occur here on assigning the new function reference for every render. Maybe the supposed performance benefits of function components counter-act my own concerns though.

Yes, I tend to avoid the class components as much as I can, I personally don't mind these "rerenderings" but I don't have any proof saying the functional components even with those functions are faster.

Should we try to set a guideline, or is it fine to have separate styles here?

Rumors say that functional component was slower in React 15, but it should be comparable in React 16, on the initial render.

There is only one prop which can trigger re-render of this component isSidebarOpened which will cause the first icon to re-render because of this dynamically created function. There is also another option here, we could create this function as 3rd param in the connect function.

I commented this already in other PR. I don't like the fact that we have a choice of the component syntax in the first place. It seems like a class component is a better choice here, but it's hard to measure performance implications. We could use mergeProps from connect more often, but this brings another set of issues to the table :)

Rumors say that functional component was slower in React 15, but it should be comparable in React 16, on the initial render.

Actually, I saw a benchmark showing that the functional components are 15% faster than the class components but unless you benchmark your self, this is irrelevant.

I'm very much in favor of avoiding premature optimization vs simpler code. And I have to admit I don't like classes that much.

With the vertical ellipsis we can move the left/right items a bit closer to the block, tighten things up. Looks better.

Feedback addressed

gziolo

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

gziolo · 2017-10-09T10:29:23Z

editor/actions.js

+ * Returns an action object used in signalling that the user toggled the sidebar
+ *
+ * @return {Object}         Action object
+ */


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

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.

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 · 2017-10-09T10:45:03Z

editor/block-settings-menu/style.scss

+}
+
+.editor-block-settings-menu__toggle {
+	transform: rotate(90deg);


Do we add spaces inside all parentheses also in CSS? I'm not sure, just checking. If yes, what has happened to you style linter? :)

We don't have a style linter?

Haha, okey :)

gziolo · 2017-10-09T10:46:55Z

editor/header/index.js

 		undo: () => dispatch( { type: 'UNDO' } ),
 		redo: () => dispatch( { type: 'REDO' } ),
-		toggleSidebar: () => dispatch( { type: 'TOGGLE_SIDEBAR' } ),
+		onToggleSidebar: () => dispatch( toggleSidebar() ),


New action creator 👍

gziolo · 2017-10-09T10:49:59Z

editor/block-settings-menu/style.scss


-	&:first-child {
-		margin-bottom: 4px;
+	.editor-block-settings-menu__content &:first-child {


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

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.

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 · 2017-10-09T12:17:25Z

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

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

youknowriad · 2017-10-09T12:21:00Z

Good catch @gziolo should be better now

gziolo

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

youknowriad force-pushed the try/block-settings-menu-toggle branch from 7e5579c to 557b507 Compare October 5, 2017 08:40

youknowriad requested review from jasmussen and mtias October 5, 2017 08:40

aduth previously requested changes Oct 5, 2017

View reviewed changes

youknowriad and others added 5 commits October 6, 2017 10:49

Blocks: Update the block's right menu with an open/close toggle

1e6e9b1

Polish the margins and positions of the menus.

269c57c

With the vertical ellipsis we can move the left/right items a bit closer to the block, tighten things up. Looks better.

Blocks: Select block when clicking the right menu toggle

df203f4

Blocks: Add a tooltip to the block settings toggle

3a3908a

Store: Factorize some action creators

2456fd2

youknowriad force-pushed the try/block-settings-menu-toggle branch from 089d0f7 to 2456fd2 Compare October 6, 2017 09:57

jasmussen mentioned this pull request Oct 9, 2017

Consider moving cog and trash to quick toolbar #2156

Closed

jasmussen mentioned this pull request Oct 9, 2017

Improve flow for inserting between blocks #2925

Closed

gziolo reviewed Oct 9, 2017

View reviewed changes

Block Settings Menu Style linting

c3b8cd1

youknowriad force-pushed the try/block-settings-menu-toggle branch from 49c670c to c3b8cd1 Compare October 9, 2017 11:51

Block Settings Menu: Fix Close Tooltip

8977595

gziolo approved these changes Oct 9, 2017

View reviewed changes

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

youknowriad merged commit 8557b53 into master Oct 9, 2017

youknowriad mentioned this pull request Oct 9, 2017

Editor: HTML Editor per block #2797

Merged

5 tasks

aduth deleted the try/block-settings-menu-toggle branch October 9, 2017 13:20

Conversation

youknowriad commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Uh oh!

codecov bot commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jasmussen commented Oct 5, 2017

Uh oh!

youknowriad commented Oct 5, 2017

Uh oh!

jasmussen commented Oct 5, 2017

Uh oh!

aduth commented Oct 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 9, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Oct 9, 2017 • 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!

gziolo commented Oct 9, 2017

Uh oh!

youknowriad commented Oct 9, 2017

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

youknowriad commented Oct 5, 2017 •

edited

Loading

codecov bot commented Oct 5, 2017 •

edited

Loading

gziolo Oct 9, 2017 •

edited

Loading

gziolo Oct 9, 2017 •

edited

Loading