Inserter: Prevent unnecessary re-renders when selecting a different block #7724

gziolo · 2018-07-05T13:15:23Z

Description

When testing one of the performance improvements I noticed that Inserter component re-renders each time something changes in the editor. This is very unexpected because it is just a button.

The improvement I propose is to move all store operations to the child component which is the one that needs those data to be fetched. The only check that is left is whether there is any block registered to make sure we don't display the inserter when there no blocks available.

It still needs to be verified how it interacts with the template lock.

How has this been tested?

Manually with Highlight Updates enabled in React devTools.

Screenshots

Before

After

Types of changes

Performance optimization.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

…lock

jorgefilipecosta · 2018-07-05T13:23:37Z

This approach is similar to what we tried in #6796. The approach was discarded because we made a call to isUnmodifiedDefaultBlock on every state change so we were not sure if that approach was better on not because isUnmodifiedDefaultBlock is a complex function. I think this PR does not contain this problem.

gziolo · 2018-07-05T13:31:30Z

@jorgefilipecosta - I think it is different, because I moved all this logic to the child component which isn't displayed by default. The only issue with this approach is that it might happen that we display the inserter when there are no blocks to insert and it will show no blocks message because of this relaxed check: https://github.com/WordPress/gutenberg/pull/7724/files#diff-922ca3021f1da894e7d2fe1e9e533335R38.

tofumatt

This makes a bunch of sense to me; we shouldn't have components whose job it is to pass state info to their child components instead of having the children use redux.

I can't speak to locking but otherwise looks good to me. Tested locally and saw similar results in Firefox with fewer re-draws.

tofumatt · 2018-07-05T23:54:15Z

editor/components/inserter/index.js

-						onClose();
-					};
-
-					return <InserterMenu items={ items } onSelect={ onSelect } rootUID={ rootUID } />;


This is nice to see removed; it's always a bit of a code smell to have a component that just passes props down to other components without using them.

tofumatt · 2018-07-05T23:54:58Z

editor/components/inserter/menu.js

+			const { index, rootUID, layout } = insertionPoint;
+			const { name, initialAttributes } = item;
+			const insertedBlock = createBlock( name, { ...initialAttributes, layout } );
+			if ( selectedBlock && isUnmodifiedDefaultBlock( selectedBlock ) ) {


I think this function is a bit mushed together and could use some whitespace/breathing room around conditionals, so I've pushed a little style tweak.

Thanks 👍

jorgefilipecosta

If we select the column block and then we try to insert inside the other column, we see the no available blocks message.

Before the insert button did not appear in this case which was also not correct. This points to a problem in our inserter where it always references the currently selected block as the inserter position. While we sometimes put the button in places where it should use a specific position different from the currently selected one. This also causes a bug where inline images may be inserted in the wrong place (but it is already present in master).

I think this seems to work great.
We are now showing the inserter all the times, even in cases where we can not obviously insert (e.g: general CPT template Lock). So we have a UX decision here cc: @karmatosed. Should we show the inserter all the time, even if no blocks are available to increase the performance? Or should we hide the inserter if we already know that the user will not be able to insert blocks?
I don't have a strong option on the best option here.

To note that I found a bug where our hiding logic of the button does not account inline blocks so if locking exists we can not insert inline images in paragraphs because we hide the inserter, this PR shows the inserter all the time so the problem gets fixed.
Technically going with this option of showing the inserter all the time is much simpler as I feel the logic will only get complex with layout blocks and other mechanisms.

I will create issues for the existing problems I noticed while testing this.

gziolo · 2018-07-06T10:12:33Z

We are now showing the inserter all the times, even in cases where we can not obviously insert (e.g: general CPT template Lock). So we have a UX decision here cc: @karmatosed. Should we show the inserter all the time, even if no blocks are available to increase the performance? Or should we hide the inserter if we already know that the user will not be able to insert blocks?

I was afraid, it might happen with all the complex logic we have for child blocks and similar concepts. Assuming we don't want to show the button when there are no blocks to pick, should we gray it out instead to keep it as part of the UI but inform the user that it isn't usable?

jorgefilipecosta · 2018-07-06T10:23:17Z

I was afraid, it might happen with all the complex logic we have for child blocks and similar concepts. Assuming we don't want to show the button when there are no blocks to pick, should we gray it out instead to keep it as part of the UI but inform the user that it isn't usable?

I guess it depends on the situation. On the global inserter, we should probably show it in gray (I mean a different gray). On the inline buttons that appear they should probably not appear if it is not possible to insert.

karmatosed · 2018-07-11T15:21:31Z

Assuming we don't want to show the button when there are no blocks to pick, should we gray it out instead to keep it as part of the UI but inform the user that it isn't usable?

Greying out is the right approach here. Where possible not seeing at all is the best but if that can't happen then lets go for background state.

aduth · 2018-09-13T19:27:14Z

Is it likely this will be resumed, or can it be closed?

gziolo · 2018-09-28T10:00:54Z

I would like to do another round of refactoring and improve the logic which decides whether the button should be rendered.

gziolo · 2018-10-30T17:09:00Z

@aduth opened a more up to date version of the same change and it's also more close to the actual implementation. Closing this one in favor of #11243.

Inserter: Prevent unnecessary re-renders when selecting a different b…

d092233

…lock

gziolo added [Feature] Inserter The main way to insert blocks using the + button in the editing interface [Type] Performance Related to performance efforts labels Jul 5, 2018

gziolo self-assigned this Jul 5, 2018

gziolo requested review from aduth, jorgefilipecosta and a team July 5, 2018 13:15

tofumatt approved these changes Jul 5, 2018

View reviewed changes

chore: Tweak whitespace to make it a bit more readable

2679073

jorgefilipecosta reviewed Jul 6, 2018

View reviewed changes

gziolo added the Needs Design Feedback Needs general design feedback. label Jul 10, 2018

karmatosed removed the Needs Design Feedback Needs general design feedback. label Jul 11, 2018

aduth added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Sep 13, 2018

gziolo closed this Oct 30, 2018

gziolo deleted the update/prevent-obsolete-inserter-rerenders branch October 30, 2018 17:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inserter: Prevent unnecessary re-renders when selecting a different block #7724

Inserter: Prevent unnecessary re-renders when selecting a different block #7724

gziolo commented Jul 5, 2018

jorgefilipecosta commented Jul 5, 2018 •

edited

Loading

gziolo commented Jul 5, 2018

tofumatt left a comment

tofumatt Jul 5, 2018

tofumatt Jul 5, 2018

gziolo Jul 6, 2018

jorgefilipecosta left a comment

gziolo commented Jul 6, 2018

jorgefilipecosta commented Jul 6, 2018

karmatosed commented Jul 11, 2018

aduth commented Sep 13, 2018

gziolo commented Sep 28, 2018

gziolo commented Oct 30, 2018

Inserter: Prevent unnecessary re-renders when selecting a different block #7724

Inserter: Prevent unnecessary re-renders when selecting a different block #7724

Conversation

gziolo commented Jul 5, 2018

Description

How has this been tested?

Screenshots

Before

After

Types of changes

Checklist:

jorgefilipecosta commented Jul 5, 2018 • edited Loading

gziolo commented Jul 5, 2018

tofumatt left a comment

Choose a reason for hiding this comment

tofumatt Jul 5, 2018

Choose a reason for hiding this comment

tofumatt Jul 5, 2018

Choose a reason for hiding this comment

gziolo Jul 6, 2018

Choose a reason for hiding this comment

jorgefilipecosta left a comment

Choose a reason for hiding this comment

gziolo commented Jul 6, 2018

jorgefilipecosta commented Jul 6, 2018

karmatosed commented Jul 11, 2018

aduth commented Sep 13, 2018

gziolo commented Sep 28, 2018

gziolo commented Oct 30, 2018

jorgefilipecosta commented Jul 5, 2018 •

edited

Loading