Refactor the hovered area component to a hook #15038

youknowriad · 2019-04-18T09:30:31Z

The idea here is that by using a hook, we don't need to pass the "wrapper" as a prop which means we don't need to "force rerender". I'm not sure this has a significant impact on performance but at least it avoids one extra re-rendering and the code is way cleaner.

Testing instructions

Make sure the block movers only show up when you hover the left area of the block (and the right in RTL languages)

youknowriad · 2019-04-18T09:31:12Z

Make sure to disable the "whitespace" changes when looking at the diff :)

packages/block-editor/src/components/block-list/hover-area.js

youknowriad · 2019-05-07T11:36:34Z

This is not blocked anymore and should be ready for a final review.

youknowriad · 2019-05-13T08:49:29Z

Would love to get a review here :)

youknowriad · 2019-05-15T10:40:03Z

Please review :P @WordPress/gutenberg-core

nicolad · 2019-05-19T15:06:57Z

packages/block-editor/src/components/block-list/hover-area.js

- hoverArea = isRTL ? 'left' : 'right';
- }
+ let newHoveredArea = null;
+ if ( ( event.clientX - left ) < width / 3 ) {


Is it possible to have width / 3 extracted into a variable? this way it would be easier to reason about this at first sight.

aduth · 2019-05-21T18:48:22Z

I have similar sentiments as to #15053 (review) when approaching this.

Would you want to apply a similar approach from there as in extracting hooks? (#15053 (comment))

youknowriad · 2019-05-21T20:03:15Z

I have similar sentiments as to #15053 (review) when approaching this.

If you're speaking about the BlockListBlock component, you're right but this PR don't touch that component much (check the diff without the "whitespaces" changes).

If you're talking about the useHoveredArea itself, IMO it's simple enough and doesn't really need splitting.

aduth · 2019-05-21T20:24:38Z

If you're speaking about the BlockListBlock component, you're right but this PR don't touch that component much (check the diff without the "whitespaces" changes).

Ah, this is what I was referring to. I didn't immediately grasp what was being changed.

youknowriad · 2019-05-30T11:49:36Z

Should we merge this?

nicolad · 2019-05-31T08:00:43Z

Based on the PR's scope this looks good.
I've started another refactor related to hooks adoption: #15934

aduth

This looks like a very nice refactor of the introduction of the hook-based useHoverArea, the flattening of BlockListBlock, and the avoidance of a forced re-render. 👍

aduth · 2019-06-06T16:05:00Z

packages/block-editor/src/components/block-list/hover-area.js

- render() {
- const { hoverArea } = this.state;
- const { children } = this.props;
+ wrapper.current.addEventListener( 'mousemove', onMouseMove );


So, a few questions for my own clarification:

Even though the wrapper passed from block.js is initialized as null using useRef, we're safe to know that it's a DOM reference, presumably by the fact that useEffect callback will occur after the initial render?

Does wrapper need to be a dependency of the useEffect or are we okay to assume that it will never change (I assume this will help us to avoid needing to unbind / rebind event handlers too?)

Even though the wrapper passed from block.js is initialized as null using useRef, we're safe to know that it's a DOM reference, presumably by the fact that useEffect callback will occur after the initial render?

yes, also took a look at some react community hooks and a lot of them use a similar technique.

Does wrapper need to be a dependency of the useEffect or are we okay to assume that it will never change (I assume this will help us to avoid needing to unbind / rebind event handlers too?)

This is the kind of questions I'm still unsure about. My naive thinking would say that leaving it out is better in terms of performance especially because we know that it doesn't change. (Same here, community hooks based on refs don't add it, at least the ones I saw).

aduth · 2019-06-06T16:05:53Z

packages/block-editor/src/components/block-list/block.js

@@ -105,13 +106,14 @@ function BlockListBlock( {
 const wrapper = useRef( null );
 useEffect( () => {
 blockRef( wrapper.current, clientId );
- // We need to rerender to trigger a rerendering of HoverArea.
- rerender();


I guess this is where we get most of the performance benefit? We're currently rendering twice for every component update?

yes, I think in my tests, the difference is not really noticeable but yeah it's way cleaner that way.

To clarify we're not rerendering on every update but we're rendering twice on mount.

It might be easier to reason about componentDidMount in a functional component with something like this:
https://github.com/streamich/react-use/blob/master/docs/useMount.md
?
shall I add this utility function ? not the whole library

I read in several places that ideally when using hooks we should try to avoid thinking in terms of lifecycle hooks (did mount, did update) and instead thinking in terms of effects as results of props changes, state changes... Makes me wonder if we want to integrate this kind of hooks.

aduth · 2019-06-06T16:09:22Z

packages/block-editor/src/components/block-list/hover-area.js


- if ( hoverArea !== this.state.hoverArea ) {


We're losing this condition in the refactor, which means we're setting state on every mousemove event?

I thought so as well, but we're not in my tests. I think this is automatically taken care of by react in its useState hook.

Also, adding this test is not a good idea, at least not without moving from useState into useReducer. When using useState it's not safe to use the "previous" state when setting a new one as the previous one might not be the exact previous state since concurrent setState calls can happen.

I thought so as well, but we're not in my tests. I think this is automatically taken care of by react in its useState hook.

Huh! That's very nice.

For my own curiosity, I found the documented reference to this behavior:

Bailing out of a state update

If you update a State Hook to the same value as the current state, React will bail out without rendering the children or firing effects. (React uses the Object.is comparison algorithm.)

https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update

youknowriad added [Type] Performance Related to performance efforts [Type] Code Quality Issues or PRs that relate to code quality labels Apr 18, 2019

youknowriad requested review from aduth, ajitbohra, ellatrix, gziolo, nerrad, noisysocks, oandregal, ntwb and talldan as code owners April 18, 2019 09:30

youknowriad force-pushed the add/use-hovered-area-hook branch from 1d91dc8 to b3052b7 Compare April 18, 2019 10:01

nerrad reviewed Apr 18, 2019

View reviewed changes

packages/block-editor/src/components/block-list/hover-area.js Show resolved Hide resolved

youknowriad force-pushed the update/block-component-hooks branch from 453dc59 to 9fd8640 Compare May 7, 2019 09:34

youknowriad changed the base branch from update/block-component-hooks to master May 7, 2019 11:22

youknowriad force-pushed the add/use-hovered-area-hook branch from b3052b7 to 28c5c95 Compare May 7, 2019 11:35

youknowriad requested a review from nerrad May 7, 2019 11:36

youknowriad force-pushed the add/use-hovered-area-hook branch 3 times, most recently from e30379f to 90e4af8 Compare May 7, 2019 14:40

nicolad reviewed May 19, 2019

View reviewed changes

Refactor the hovered area component to a hook

4b513dd

youknowriad force-pushed the add/use-hovered-area-hook branch from 90e4af8 to 4b513dd Compare June 6, 2019 11:54

aduth approved these changes Jun 6, 2019

View reviewed changes

aduth requested changes Jun 6, 2019

View reviewed changes

aduth approved these changes Jun 6, 2019

View reviewed changes

youknowriad merged commit 2564c21 into master Jun 6, 2019

youknowriad deleted the add/use-hovered-area-hook branch June 6, 2019 17:27

youknowriad added this to the Gutenberg 5.9 milestone Jun 7, 2019

senadir mentioned this pull request Aug 12, 2019

Cannot insert Heading component into Cover #17013

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor the hovered area component to a hook #15038

Refactor the hovered area component to a hook #15038

youknowriad commented Apr 18, 2019 •

edited

Loading

youknowriad commented Apr 18, 2019

youknowriad commented May 7, 2019

youknowriad commented May 13, 2019

youknowriad commented May 15, 2019

nicolad May 19, 2019

aduth commented May 21, 2019

youknowriad commented May 21, 2019

aduth commented May 21, 2019

youknowriad commented May 30, 2019

nicolad commented May 31, 2019

aduth left a comment

aduth Jun 6, 2019

youknowriad Jun 6, 2019

aduth Jun 6, 2019

youknowriad Jun 6, 2019

youknowriad Jun 6, 2019

nicolad Jun 6, 2019 •

edited

Loading

youknowriad Jun 7, 2019

aduth Jun 6, 2019

youknowriad Jun 6, 2019

aduth Jun 6, 2019

Refactor the hovered area component to a hook #15038

Refactor the hovered area component to a hook #15038

Conversation

youknowriad commented Apr 18, 2019 • edited Loading

youknowriad commented Apr 18, 2019

youknowriad commented May 7, 2019

youknowriad commented May 13, 2019

youknowriad commented May 15, 2019

Choose a reason for hiding this comment

aduth commented May 21, 2019

youknowriad commented May 21, 2019

aduth commented May 21, 2019

youknowriad commented May 30, 2019

nicolad commented May 31, 2019

aduth 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

nicolad Jun 6, 2019 • edited Loading

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

Bailing out of a state update

youknowriad commented Apr 18, 2019 •

edited

Loading

nicolad Jun 6, 2019 •

edited

Loading