Block List: Display inserter button after block on hover, focus #2890

aduth · 2017-10-05T18:38:25Z

Closes #2752
Related: #833

This pull request seeks to display a block inserter after a hovered or focused block, enabling a user to easily insert between blocks. This is a first step toward eventually dropping the header global inserter (#2755).

Design notes:

This is a best-attempt design. Iterations or suggestions are welcome. In particular, the specifics of the clickable/tappable area and hover bounds may need some improvement.

Testing instructions:

Verify that you can insert after a block which is focused or hovered, and that regressions do not otherwise occur with the global inserter or end-of-content inserters.

Navigate to Gutenberg > New Post
Insert a paragraph
Note that hovering this paragraph exposes no inserter, since it's the last block (expected to use end-of-content inserter)
Insert a second block
Hover and/or focus the first paragraph inserted in step 2
Note that an inserter is shown, enabling you to insert a new block after the paragraph

codecov · 2017-10-05T18:38:31Z

Codecov Report

Merging #2890 into master will increase coverage by 0.03%.
The diff coverage is 36.73%.

@@            Coverage Diff             @@
##           master    #2890      +/-   ##
==========================================
+ Coverage   34.07%   34.11%   +0.03%     
==========================================
  Files         192      192              
  Lines        5675     5710      +35     
  Branches      996     1004       +8     
==========================================
+ Hits         1934     1948      +14     
- Misses       3165     3181      +16     
- Partials      576      581       +5

Impacted Files	Coverage Δ
editor/actions.js	`38.88% <0%> (-2.29%)`	⬇️
editor/modes/visual-editor/block.js	`0% <0%> (ø)`	⬆️
editor/inserter/index.js	`0% <0%> (ø)`	⬆️
editor/reducer.js	`89.18% <100%> (+0.22%)`	⬆️
editor/selectors.js	`96.89% <100%> (+0.14%)`	⬆️
editor/inserter/menu.js	`49.19% <25%> (-0.4%)`	⬇️
components/dropdown/index.js	`69.56% <37.5%> (-17.11%)`	⬇️
editor/modes/visual-editor/inserter.js	`92.85% <50%> (-7.15%)`	⬇️

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 1400611...e3e0741. Read the comment docs.

youknowriad

Great job so far.

I noticed a small "glitch" which is not totally related but I think we this change, we have to update the inserter at the bottom of the editor to always insert at the end of the post instead of after the selected block.

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

editor/inserter/index.js

+		const { opened } = nextState;
+		if (
+			( this.state.opened !== opened || this.props.insertIndex !== insertIndex ) &&
+			Number.isInteger( insertIndex )


When is this second check necessary?

The global and end-of-content inserters don't specify the insertIndex prop, so this ensures the sibling insertion point is only set for the between-inserters. It might be the case that insertIndex !== undefined could work well here. I used Number.isInteger elsewhere to capture 0 but not null (since JavaScript is a very sensible language where null >= 0 evaluates to true 😆 )

youknowriad · 2017-10-06T08:08:39Z

editor/inserter/menu.js

@@ -51,11 +51,11 @@ export class InserterMenu extends Component {
 	}

 	componentDidMount() {
-		document.addEventListener( 'keydown', this.onKeyDown );
+		document.addEventListener( 'keydown', this.onKeyDown, true );


Could you explain why this change is necessary?

This is a similar issue to the one discussed at #2706: Namely, WritingFlow is handling the arrow presses and moving between blocks. The true argument allows us to handle the key press earlier during the capture phase. I might take another pass at this, and I wonder why we need to bind to the document at all instead of using React events (since I expect bubbling would work better in the latter case). If I keep it as-is, I'll add a clarifying comment.

youknowriad · 2017-10-06T08:11:49Z

editor/modes/visual-editor/block.js


-		if ( isHovered || isSelected || isMultiSelected ) {
+		if ( isHovered || isSelected || isMultiSelected || isInserterOpen ) {


I guess this check is to avoid "removing" the inserter when we hover out of the block?
Maybe we can handle this in a simpler way: select the block when we open its inserter.
I'm just trying to avoid this dependency between the inserter and this component.

Maybe we can handle this in a simpler way: select the block when we open its inserter.

That sounds like a good idea! I'll give it a try.

gziolo · 2017-10-06T13:55:55Z

@aduth, thank you for working on this one. In my opinion, this was the biggest UX issues I discovered so far in Gutenberg 🥇

aduth · 2017-10-06T14:59:37Z

I noticed a small "glitch" which is not totally related but I think we this change, we have to update the inserter at the bottom of the editor to always insert at the end of the post instead of after the selected block.

Yep, makes sense. I'll change this.

gziolo · 2017-10-09T08:32:08Z

I tested this PR. I love the changes introduced ❤️

Just saying that this toolbar doesn't play well with this change. I think that the toolbar needs some rethinking, because it also makes it harder to read the previous paragraph /cc @jasmussen

The last block looks a bit different, but I think it makes a lot of sense:

I can confirm that the last commit fixes issue mentioned by @youknowriad and the add code itself look good to me.

gziolo · 2017-10-09T08:36:01Z

@jasmussen what if we would move the very last + button to be rendered the same way as all other icons, but it would be always visible? We could also mirror the behavior of the existing + button and show on hover the following:

youknowriad · 2017-10-09T08:37:40Z

editor/inserter/index.js

-						name,
-						insertionPoint
-					);
+class Inserter extends Component {


I guess we don't need this to be a class component, but probably the same discussion for/against inline props.

I hope we will see the day where there is only one supported way of creating components in React. I feel like some components are converted back and forth whenever requirements change.

youknowriad

For me, this is good to ship. There are some UI concerns like pointed out by @gziolo but worth a try. Also, I'll explore the fixed toolbar switch soon which could solve the toolbar issue.

gziolo · 2017-10-09T08:46:59Z

Yes, totally agree we should ship it as it is and continue the discussion here.

jasmussen · 2017-10-09T08:59:53Z

Designwise, I'd like to echo @gziolo's comments, this isn't ideal and it adds some visual load to the block silhuette.

I don't want to block this from moving forward — nice work — but it does seem like a half-step.

I understand the moving parts — it has to be accessible, it has to be fairly simple codewise, and the previous solution wasn't great either. But interface wise, we are at a point where we should be extremely careful to introduce visual regressions, especially since this is one of the critiques that is (fairly so) leveraged on Gutenberg these days.

One of the design patterns of Gutenberg is that it's possible to show further block options when a block is selected. A common editor pattern when you want to insert content in between paragraphs or other elements, is to make a linebreak.

Perhaps we could show the inserter when a block is selected? Perhaps we could show an empty line when a block is selected, and have the Enter button put the cursor there? Perhaps both? It seems like it is worth looking at how desktop publishing apps solve this.

Either way it's important we strike a balance between something that is easy to implement and solves the problem, with the cognitive load that it adds. In this case it feels like there's an imbalance. This implementation also doesn't solve the inserting images inline (#2043), and it seems like this inserter pattern would've been a good opportunity to look at that.

gziolo · 2017-10-09T09:28:39Z

As I mentioned above, not having such button available next to each block caused me the biggest struggle when I opened Gutenberg for the first time.

I must admit, I got to easily tempted to click Merge button in here :) I will be more cautious next time and wait for a wider feedback whenever we introduce UI/UX changes.

Perhaps we could show the inserter when a block is selected? Perhaps we could show an empty line when a block is selected, and have the Enter button put the cursor there? Perhaps both? It seems like it is worth looking at how desktop publishing apps solve this.

At the moment when you hover over one of the existing blocks you can see arrows which allow moving the block up or down in the document hierarchy:

In that context add block button plays a similar function, but as you observed it adds another visual element to the block. Let's continue the discussion in #2925.

mtias · 2017-10-09T10:15:27Z

Adding a bit more context as I discussed this one with @aduth in person. The point was to add functionality that allowed the user to insert blocks in between other blocks easily and without hidden interactions.

Implementation wise I think we should treat this inserter as a separate element—or at least, visually separate. It should show when we hover the limit area between two blocks. Additionally, it can show up when a block is selected. Something more like this:

This is also in anticipation of nested blocks, which would use the pattern of the bottom inserter.

aduth · 2017-10-09T12:57:06Z

Implementation wise I think we should treat this inserter as a separate element—or at least, visually separate. It should show when we hover the limit area between two blocks.

In trying to imagine how this would operate in practice, I get the sense that it might be frustrating to find the sweet spot between hovering and individual block and hovering between blocks to reveal the inserter. It was already not-great in the implementation which was proposed here.

aduth added the [Feature] Inserter The main way to insert blocks using the + button in the editing interface label Oct 5, 2017

youknowriad reviewed Oct 6, 2017

View reviewed changes

aduth added 4 commits October 6, 2017 11:00

Inserter: Remove unused ESLint inline configuration

7e0f2e3

Inserter: Show inserter between blocks on hover, focus

4708e7c

Inserter: Select block when opening hover inserter

ac2888c

Inserter: Append end-of-content inserter block to end

e3e0741

aduth force-pushed the update/between-inserter branch from 5bc749e to e3e0741 Compare October 6, 2017 15:58

youknowriad reviewed Oct 9, 2017

View reviewed changes

youknowriad approved these changes Oct 9, 2017

View reviewed changes

gziolo merged commit b6480da into master Oct 9, 2017

gziolo deleted the update/between-inserter branch October 9, 2017 08:47

jasmussen mentioned this pull request Oct 9, 2017

Improve flow for inserting between blocks #2925

Closed

gziolo restored the update/between-inserter branch October 9, 2017 11:23

gziolo deleted the update/between-inserter branch October 9, 2017 11:23

gziolo mentioned this pull request Oct 9, 2017

Revert "Block List: Display inserter button after block on hover, focus" #2929

Merged

This was referenced Oct 9, 2017

Block List: Display inserter button after block on hover, focus #2752

Closed

Block List: Display inserter button between blocks #3008

Merged

Inserter: Remove "global" (toolbar) inserter #2755

Closed

aduth mentioned this pull request Jan 10, 2018

Blocks: Reduce sibling inserter initial height #4384

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block List: Display inserter button after block on hover, focus #2890

Block List: Display inserter button after block on hover, focus #2890

aduth commented Oct 5, 2017

codecov bot commented Oct 5, 2017 •

edited

Loading

youknowriad left a comment

youknowriad Oct 6, 2017

aduth Oct 6, 2017

youknowriad Oct 6, 2017

aduth Oct 6, 2017

youknowriad Oct 6, 2017

aduth Oct 6, 2017

gziolo commented Oct 6, 2017

aduth commented Oct 6, 2017

gziolo commented Oct 9, 2017

gziolo commented Oct 9, 2017

youknowriad Oct 9, 2017

gziolo Oct 9, 2017

youknowriad left a comment

gziolo commented Oct 9, 2017

jasmussen commented Oct 9, 2017

gziolo commented Oct 9, 2017 •

edited

Loading

mtias commented Oct 9, 2017

aduth commented Oct 9, 2017


		if ( isHovered \|\| isSelected \|\| isMultiSelected ) {
		if ( isHovered \|\| isSelected \|\| isMultiSelected \|\| isInserterOpen ) {

Block List: Display inserter button after block on hover, focus #2890

Block List: Display inserter button after block on hover, focus #2890

Conversation

aduth commented Oct 5, 2017

codecov bot commented Oct 5, 2017 • edited Loading

Codecov Report

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Oct 6, 2017

aduth commented Oct 6, 2017

gziolo commented Oct 9, 2017

gziolo commented Oct 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

gziolo commented Oct 9, 2017

jasmussen commented Oct 9, 2017

gziolo commented Oct 9, 2017 • edited Loading

mtias commented Oct 9, 2017

aduth commented Oct 9, 2017

codecov bot commented Oct 5, 2017 •

edited

Loading

gziolo commented Oct 9, 2017 •

edited

Loading