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: Try adding the left/right block hover areas #6101

Merged
merged 7 commits into from Apr 12, 2018

Conversation

Projects
None yet
5 participants
@youknowriad
Contributor

youknowriad commented Apr 10, 2018

For unselected blocks, we only show movers the movers when we're close to them and same for the block settings menu.

Testing instructions

  • Add several blocks
  • Try hovering next to the left and the right area of the blocks
  • When you're close to the left the movers appear
  • When you're close to the right the settings menu appear

closes #3044

@youknowriad youknowriad self-assigned this Apr 10, 2018

@youknowriad youknowriad requested review from mtias and jasmussen Apr 10, 2018

Block: Adding the left/right block hover areas
Show movers on the left and settings on the right
constructor() {
super( ...arguments );
this.state = {
hoverArea: null,

This comment has been minimized.

@youknowriad

youknowriad Apr 10, 2018

Contributor

I can easily add an isHovered state to this component to remove it from the block.js component but it seems that this state is slightly more complex than just keeping a boolean on whether the block is hovered or not. Might be good to do but I'll defer to @aduth here. I think you have more knowledge about the edge cases there.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 10, 2018

Too cool.

It seems like it would be worth trying a few things to tweak this further. That is, always hide the mover and ellipsis, even when the block is selected. Perhaps combined with the hoverable area left and right being wider, so they invoke more easily, that will strike a nice balance.

Can we also add a transition? Something alongthe lines of transition: opacity .4s linear, so they side UI fades in when you enter the hover area?

We want to make sure this works well on mobile also. That is, on mobile, these shouldn't fade out at all, as the side UI sits below the block in the mobile breakpoints.

@mtias

This comment has been minimized.

Contributor

mtias commented Apr 10, 2018

Thanks for trying this. It's feeling rather nice to me and greatly reduces the UI weight. If adding this also for the selected state we have to make sure things can be targeted with keyboard.

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

Updated the PR. It works also for selected blocks, but the elements are tabbable even if hidden so it works fine using keyboard.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 11, 2018

Here's a GIF:

hover areas

This seems to work really well for me now, very very nice.

I feel like we should make the hoverable area to reveal the side controls bigger... It feels like it's below 20% now, but it could be between 25 and 33% the width of the whole block, I feel like.

Or is it measured in pixels right now? If yes, then the breakpoint where we switch between mobile side UI and desktop side UI is 600px, and so if we can't use percent, it might be worth trying to make it 200px left side, 200px neutral center, 200px right side. There's no reason not to go pretty big with this stuff, at least for a start, we can always pull back.

Nice!

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Apr 11, 2018

@jasmussen So right now the hover area is 120px and when the block is smaller than 2* 120px the hover area is 50% of the block. I can increase it to 200px or use a percentage, your choice :)

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 11, 2018

Let's try, and if to make the areas min-width: 160px, max-width: 240px. Or width: 30%. Either of those, I think, would be good to test as a start, and then we can tune as we go. Sound good?

@mtias

This comment has been minimized.

Contributor

mtias commented Apr 11, 2018

I'm really liking this, let's get it in. :)

@@ -17,4 +17,16 @@
animation: slide_in_right 0.1s forwards;
}
@mixin fade_in {
animation: fade-in 0.5s ease-out;

This comment has been minimized.

@mtias

mtias Apr 11, 2018

Contributor

I think we can make this slightly faster, 0.3 or 0.4.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 12, 2018

Dig it. This really lightens the cognitive burden on the UI, while still allowing powerful tools to be present. To echo Matías, we should definitely try this. And this fixes #3044.

Also appreciate bundling the hover label with the side hover, I think that's perfectly good to try. 👍 👍 from me.

@youknowriad youknowriad merged commit 503ddba into master Apr 12, 2018

2 checks passed

codecov/project 44.4% (-0.08%) compared to 9f473f4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/block-hover-areas branch Apr 12, 2018

@youknowriad youknowriad added this to the 2.7 milestone Apr 12, 2018

}
onFocus( event ) {
this.setState( {

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

Why was this needed?

This comment has been minimized.

@youknowriad

youknowriad Apr 12, 2018

Contributor

In the breadcrumb component at the moment no because it's not shown at all when the block is selected, but I didn't want to write the implementation depending on something not in the component itself. If tomorrow we decide to show this breadcrumb even on selected blocks, this will become necessary.

}
onMouseMove( event ) {
const { width, left, right } = this.container.getBoundingClientRect();

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

What forces layout / reflow

[...] This is also called reflow or layout thrashing, and is common performance bottleneck.

Box metrics

  • [...] elem.getBoundingClientRect()

https://gist.github.com/paulirish/5d52fb081b3570c81e3a

And in a mousemove, it's likely being called hundreds of times 🙁

This comment has been minimized.

@youknowriad

youknowriad Apr 12, 2018

Contributor

I guess I can look for alternatives to getBoundingClientRect

This comment has been minimized.

@youknowriad

youknowriad Apr 12, 2018

Contributor

Can't think of any alternative without browser reflow

}
componentDidMount() {
// Disable reason: We use findDOMNode to avoid unnecessary extra dom Nodes

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

Was this the main reason for using mousemove instead of some floating invisible elements which trigger on mouseenter?

This comment has been minimized.

@youknowriad

youknowriad Apr 12, 2018

Contributor

No, the main reason is that mouseenter doesn't trigger on areas with lower z-index, since these areas would need a lower z-index to allow writing in the editables etc...

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

Yeah, I see what you mean. I played with some ideas for creating an inverted hover area "hole" to trigger the unhover while still allowing the contents to be interacted, but it's certainly more complex. Maybe worth exploring, but I struggled to get it working perfectly (particularly for the "hole" to be interacted with, and capturing all unhovers with various layering contexts).

invert-hover

Diff: https://gist.github.com/aduth/d08fe307491f641f333fcd362ef38bf5

*/
import { Component, findDOMNode } from '@wordpress/element';
const withHoverAreas = ( WrappedComponent ) => {

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

Should we have used @wordpress/element's createHigherOrderComponent ?

@@ -699,3 +699,11 @@
padding-top: 6px;
}
}
.editor-block-breadcrumb {

This comment has been minimized.

@aduth

aduth Apr 12, 2018

Member

Aside: I applied an invalid class name per standards; this should be .editor-block-list__breadcrumb

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Apr 13, 2018

Wanted to note that the use of a keyframe animation can cause some jumpiness. I will, in an incoming PR, refactor this to use just a transition, which should fix that.

<BlockSettingsMenu
uids={ [ uid ] }
rootUID={ rootUID }
renderBlockMenu={ renderBlockMenu }
isHidden={ ! isHovered || hoverArea !== 'right' }

This comment has been minimized.

@iseulde

iseulde Apr 26, 2018

Member

@youknowriad Why does the toolbar only hide? Why not prevent rendering? If there's a reason, it's not immediately clear from this code.

This comment has been minimized.

@youknowriad

youknowriad Apr 26, 2018

Contributor

Because it should stay tabbable. When it receives the focus it's shown.
There's a comment above (with a typo :P)

We render block movers and block settings to keep them tabbale even if hidden

This comment has been minimized.

@iseulde

iseulde Apr 26, 2018

Member

Oh, thanks, I didn't read that far up. :)

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