Skip to content
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

Block List: Display inserter button between blocks #3008

Merged
merged 2 commits into from Oct 16, 2017

Conversation

Projects
None yet
3 participants
@aduth
Copy link
Member

commented Oct 12, 2017

Previously: #2890
Related: #2983

This pull request seeks to display a block inserter between blocks by hover or tab focus, enabling a user to easily insert between blocks. Originally this was introduced in #2890 and later reverted in #2929 pending a design review. This is a first step toward eventually dropping the header global inserter (#2755).

The changes here use the design reference from #2983:

insert-between

As implemented:

insert-between

Implementation notes:

Since hovering in the specific area between blocks with a cursor can require a difficult amount of precision, I used a technique to provide a buffer for the cursor while the inserter is visible. A similar example of this sort of cursor intent buffer can be seen in this article about Amazon.com's menu behavior. Consider the following visualization of the effect:

hover-range

Changes include a straight revert of the revert in #2929, plus additional changes per design recommendations.

I chose to implement the sibling inserter as only rendering the underlying inserter component while it should be considered visible. This added some complexity to the implementation, but was in an effort to reduce the number of nodes on the page at a given time. I was (and still do) experience significant lag when inserting between on the demo content. It's not clear whether this is an issue specific to these changes, and further profiling is required.

Testing instructions:

Verify that you can insert between blocks, either by hovering between blocks or tabbing from a previous block, and that regressions do not otherwise occur with the global inserter or end-of-content inserters.

  1. Navigate to Gutenberg > New Post
  2. Note that no between-serters are available while no blocks are present
  3. Insert a paragraph
  4. Note that hovering before or after the new paragraph block reveals between-serters
    • Similarly for tabbing from paragraph

Some trickier behaviors to try:

  • Pressing escape from the inserter should return focus to the inserter button and it should remain visible
  • Moving the cursor outside the between-serter region while the inserter is open should not close the inserter
  • Clicking outside the inserter without making a selection should cause the between-serter to become hidden
@codecov

This comment has been minimized.

Copy link

commented Oct 12, 2017

Codecov Report

Merging #3008 into master will decrease coverage by 0.14%.
The diff coverage is 20.68%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3008      +/-   ##
==========================================
- Coverage    32.9%   32.76%   -0.15%     
==========================================
  Files         202      203       +1     
  Lines        5883     5951      +68     
  Branches     1040     1052      +12     
==========================================
+ Hits         1936     1950      +14     
- Misses       3330     3375      +45     
- Partials      617      626       +9
Impacted Files Coverage Δ
editor/actions.js 33.33% <0%> (-1.67%) ⬇️
editor/inserter/index.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/block-list.js 0% <0%> (ø) ⬆️
editor/modes/visual-editor/sibling-inserter.js 0% <0%> (ø)
editor/reducer.js 87.74% <100%> (+0.24%) ⬆️
editor/selectors.js 95.4% <100%> (+0.19%) ⬆️
editor/modes/visual-editor/inserter.js 93.75% <100%> (+0.41%) ⬆️
editor/inserter/menu.js 49.19% <25%> (-0.4%) ⬇️
components/dropdown/index.js 70.83% <37.5%> (-16.67%) ⬇️
... and 1 more

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 385b1b9...7784c0f. Read the comment docs.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

This is really really nice. It doesn't feel heavy the way the previous implementation did.

Visually we should tweak this one:

screen shot 2017-10-13 at 09 28 48

Can we move it down like 2px? Probably that's sufficient. It will also work better if/when we try the visual tweaks from #2983.

This one is also iffy:

screen shot 2017-10-13 at 09 26 53

Can we do something like this?

expanding

That is, animate the blue line left and right of the hover inserter, instead of showing the blue insertion line?

We'd still want to show it when inserting from the toolbar button.

Edit: Also, that animation is half-baked, it should be faster and nicer and eased and so.

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

@jasmussen Thanks for the feedback and ideas.

Visually we should tweak this one: Can we move it down like 2px?

Yes, but worth noting that as implemented, it is perfectly centered between the two blocks, since there's a 4px gap, 1px block border, and 2px line height on the between-serter. Moving it further down works well if we consider all adjacent block insertions to occur after a block, but the approach I had taken gives equal weight to the intent of inserting between or before. Considering a user might want to insert a block before another one, how does this work as they try to move their cursor to this between spot?

Maybe rather than considering these inserters between blocks, we consider them as before/after the currently hovered or focused block, in which case it becomes more reasonable to shift them further away from the block boundaries.

This one is also iffy: Can we do something like this?

I think steps here could potentially depend on if we want to move forward with #2755. With the current implementation here, the line drawn is using the same underlying state for between-serters and the global inserter. Implementing your proposed line animation would be dramatically simpler if we do decide to the remove the global inserter, since it could be refactored specific to the between-serters.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Yes, but worth noting that as implemented, it is perfectly centered between the two blocks, since there's a 4px gap, 1px block border, and 2px line height on the between-serter. Moving it further down works well if we consider all adjacent block insertions to occur after a block, but the approach I had taken gives equal weight to the intent of inserting between or before.

Great point. I think this is something that we (read I) should (read will) take a look at separately. There are spacing and padding adjustments I've been meaning to look at regardless.

I think steps here could potentially depend on if we want to move forward with #2755. With the current implementation here, the line drawn is using the same underlying state for between-serters and the global inserter. Implementing your proposed line animation would be dramatically simpler if we do decide to the remove the global inserter, since it could be refactored specific to the between-serters.

Good point. I know the context for that one, and why it's being pursued, though I'm unconvinced it's a good idea, but I will post notes on that separately.

In the mean time is it possible to tweak the design of the blue line, so it is 2px tall and lies right under where the hovered plus would be?

Then when you just hover the plus it would look like your design:

hover-plus

And when you hover a block in the invoked inserter, you'd get the blue line underneath:

block-hovered

No need to animate — but they'd stack in a convenient way

@aduth aduth force-pushed the add/between-inserter-2 branch 2 times, most recently from a6e5d99 to 6477a7a Oct 13, 2017

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

Rebased to resolve conflicts, centered inserter menu below button and restyled insertion point placeholder to overlap (underlap?) with the between-serter.

I'm noticing that there's a flicker which occurs on adjacent blocks when hovering over block options in the inserter menu. Might be related to lag issues I mentioned in the original comment. Will investigate.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Visually this looks solid now, very nice.

I didn't notice this when I tested on the brand new computer in the office, but on my old underpowered 2015 Mbp 13, the blue line now appears very slowly on hover:

slowdown

It reminds me of an issue @youknowriad had for the image drag and drop, which he fixed. Are you seeing this?

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

@jasmussen Yeah, I noticed the lag as well. I believe it's related to adding the inserter element to the block list, and React trying to reconcile the entire list. In 82c3e27 I did some refactoring which remedies both the flicker I noticed and the latency as well. Let me know if it's any better for you.

I'm still noticing some lag when performing general operations like removing a block from demo content. I think we'll need to take a proper pass at block list rendering, but it does seem to have regressed in this pull request. Will continue to investigate.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

I can confirm that the lag is completely gone, when using the inserter. And also that deleting a block is now slow, in this branch, not in master. :|

Impressive work, though!

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

Okay, should be further improved now in 069f8d2 . Seems the rendering of nested arrays was very non-performant.

@aduth aduth requested a review from youknowriad Oct 13, 2017

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 13, 2017

With the current implementation here, the line drawn is using the same underlying state for between-serters and the global inserter. Implementing your proposed line animation would be dramatically simpler if we do decide to the remove the global inserter, since it could be refactored specific to the between-serters.

Noting that I did some of this refactoring already in 82c3e27, so this hinges less on a decision around global inserters.

@jasmussen

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2017

Damn impressive work. Can confirm that the hover, and the delete button, both are now super performant. Stellar work. Experiencewise, this is a 👍 👍 from me.

isBlockInsertionPointVisible,
} from '../../selectors';

class VisualEditorSiblingInserter extends Component {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 16, 2017

Contributor

Nice to see this extracted to its own component.

@@ -440,3 +440,60 @@
box-shadow: none;
}
}

.editor-visual-editor__sibling-inserter {

This comment has been minimized.

Copy link
@youknowriad

youknowriad Oct 16, 2017

Contributor

I think we should create separate folders for these components to avoid having a giant stylesheet. It's not the first one though, so might be kept for later.

This comment has been minimized.

Copy link
@aduth

aduth Oct 16, 2017

Author Member

I think we should create separate folders for these components to avoid having a giant stylesheet. It's not the first one though, so might be kept for later.

It might be more a signal that the component itself could do for a reorganization. The way I've approached it so far is: anything other than ./index.js is a separate but dependent component, and style.scss covers both the base and these dependent components. If we're able to factor out some of these subcomponents to be independent, they would have their own stylesheets.

Elsewhere I've taken the approach which might be what you're suggesting, which is to create stylesheets specific to individual components, but I found (and had received feedback) that this became sprawling and disorganized. Example:

https://github.com/Automattic/wp-calypso/tree/master/client/my-sites/media-library

@youknowriad
Copy link
Contributor

left a comment

Code wise, this is great.

I don't know exactly why, I still slightly prefer the first iteration in term of experience but happy to follow the designers choice.

aduth added some commits Oct 12, 2017

Block List: Display sibling inserter between blocks
Block List: Render inserter only while hovered, focused

Inserter: Style insertion point below between-serter

Inserter: Position between-serter as centered below

Block: Refactor insertion point within sibling inserter

Block List: Accumuate block list by Array#reduce

Significant performance degredation when children as tuple. Instead, flatten into single array

Inserter: Disallow container tabbable when visible

Prevents shift-tab focus trap which can occur while button is focused, since focusFirstTabbable would continue to keep button focused

@aduth aduth force-pushed the add/between-inserter-2 branch from 80023c1 to 7784c0f Oct 16, 2017

@aduth

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2017

I took this for a spin on mobile and it wasn't a great experience. Not entirely specific to these changes, and mostly falls under #2691. I did observed a few other issues I'll file separately relating to the icon button and ellipsis additional controls.

@aduth aduth merged commit 588ccd3 into master Oct 16, 2017

3 checks passed

codecov/project 32.76% (-0.15%) compared to 385b1b9
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the add/between-inserter-2 branch Oct 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.