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

SPT: immediately render modal with empty thumbnails #35713

Closed
wants to merge 14 commits into from

Conversation

retrofox
Copy link
Contributor

@retrofox retrofox commented Aug 22, 2019

Changes proposed in this Pull Request

This PR changes slightly the approach in how the thumbnails are dynamically previewed.

Although it doesn't depend strictly on WordPress/gutenberg#17242, it will improve reduce the previewing time even for all thumbnails and as well as the large preview (100ms)

Overview

The current behavior waits to get the templates parsed before to start to render and mount the components. It produces a delay when the template modal appears. In order to improve the performance and the user perception, I purpose the following more important changes in this PR:

  • Disengage the parsing process from the react component, starting to process as fast as possible without waiting for the component lifecycle.

  • Do not update the state with the parsed templates blocks. Update the state seems to take considerable time.

  • Add a templates-parser.js which acts as a data store and global event dispatcher. It contains selectors in order to get the desired data.

  • Dispatch onTemplateParse global event to communicate with the component. The event dispatcher sends all data about templates in the event object argument:

window.addEventListener( 'onTemplateParse', event => console.log ( event ) );

image

The event detail object contains the following fields:

  • blocks: list of the parsed blocks
  • count: the number of blocks
  • isEmpty: true is the template doesn't have content.
  • isParsing: defines if the template is being parsed or not.
  • title: template title.
  • slug: template slug.

Testing instructions

Apply the patch and confirm that you see the spinners in the thumbnails when the templates are being parsed.

Static vs Dynamic

Tests

Parsing One By One
time - blank: 0.005126953125ms
time - about: 46.718017578125ms
time - contact: 19.9072265625ms
time - portfolio: 28.283935546875ms
time - services: 51.375ms
time - team: 37.27197265625ms
time - blog: 29.65673828125ms
time - menu: 51.2978515625ms
time - total: 6304.18798828125ms

@retrofox retrofox requested review from a team as code owners August 22, 2019 20:18
@matticbot
Copy link
Contributor

@@ -26,6 +26,10 @@ const TemplateSelectorItem = props => {
blocks = [],
} = props;

if ( ! blocks.length && 'blank' !== value ) {
Copy link
Member

@obenland obenland Aug 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather then checking for blocks here, how do you feel about testing this.state.blocks in PageTemplateModal.render()?

if ( ! this.state.isOpen || isEmpty( this.state.blocks ) ) {
	return null;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think the component should function when the props aren't provided as expected. It makes them more resilient when moved to an alternative use case.

This doesn't preclude your change happening as well.

@matticbot
Copy link
Contributor

This PR does not affect the size of JS and CSS bundles shipped to the user's browser.

Generated by performance advisor bot at iscalypsofastyet.com.

@retrofox retrofox changed the title SPT: do not render empty buttons if there are not blocks SPT: render immediately modal with empty thumbnails Aug 23, 2019
@retrofox retrofox requested review from apeatling and a team August 23, 2019 13:09
@retrofox retrofox force-pushed the update/spt-rendering-template-item branch from 0e3e834 to f35651c Compare August 23, 2019 13:19
Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Async rendering these is a massive win. Great workl

@retrofox retrofox force-pushed the update/spt-rendering-template-item branch from 3bb6a5e to 1d7b508 Compare August 26, 2019 20:30
@retrofox retrofox force-pushed the update/spt-rendering-template-item branch 4 times, most recently from 5ec9a94 to 50b024c Compare September 4, 2019 13:59
@obenland obenland added this to the Ajax: Iteration 9 milestone Sep 23, 2019
@obenland obenland changed the title SPT: render immediately modal with empty thumbnails SPT: immediately render modal with empty thumbnails Sep 25, 2019
@retrofox retrofox force-pushed the update/spt-rendering-template-item branch from 9fd07ab to ae8adf1 Compare September 27, 2019 16:34
@obenland
Copy link
Member

Page load times are still extremely long. Locally it takes 9s to fully load the page, including the thumbnail previews. With static thumbnails it's 3s (which is still long).

I'm afraid we'll need to do better before this is mergeable. :/

@obenland
Copy link
Member

@youknowriad Do you happen to have any ideas how we can cut down on the time it takes to setState here?

Screen Shot 2019-09-30 at 10 53 07 AM

@retrofox
Copy link
Contributor Author

Locally it takes 9s to fully load the page,

it doesn't take that time to me. Let me paste a gif with a clock at the bottom right corner.

dynamic-preview-02

@retrofox
Copy link
Contributor Author

I've measured both process. The reduce loop wich contemplates parsing all block of all templates: time:reduce: 351.2021484375ms,
and setting the state: time:set-state: 1388.221923828125ms

@retrofox
Copy link
Contributor Author

Another thing I forgot to mention that is I'm testing this PR together with this one, reducing the delay before to start to compute the scale factor from 100ms to 0, through of the __exoperimentalScalingDelay property.

@youknowriad
Copy link
Contributor

@youknowriad Do you happen to have any ideas how we can cut down on the time it takes to setState here?

I assume that's the setState that sets the block list and triggers the rendering. I fear that there's not much we can do there. things to consider maybe:

  • Cut the number of blocks to render (or maybe render some and then update the list with the full list async)
  • In Core an idea to explore is a "preview mode" for blocks where maybe somethings can be removed entirely, components only useful for edit like block appenders, movers, dropzones... (it needs to be measured though to avoid unnecessary complexity)

@retrofox
Copy link
Contributor Author

retrofox commented Oct 1, 2019

Preview mode is something that worths to dive imo. We talked about this a few weeks ago.
What I've done, just for this approach, is avoiding to write the state with the parsed blocks. Going to update the PR description.

@retrofox retrofox force-pushed the update/spt-rendering-template-item branch 2 times, most recently from d1ac5a0 to 653f7ff Compare October 3, 2019 00:43
@lancewillett
Copy link
Contributor

Cleaning up older pull requests as part of a GitHub scrub.

Is this change still valid? What's needed to move this along? It looks like an important performance boost. CC @retrofox

If not valid, please close with a comment. Thanks!

@apeatling apeatling removed their request for review July 21, 2020 20:03
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2020
@sarayourfriend sarayourfriend changed the base branch from master to trunk November 20, 2020 16:14
@github-actions
Copy link

github-actions bot commented May 5, 2021

This PR has been marked as stale due to lack of activity within the last 30 days.

@obenland
Copy link
Member

There's a new version of SPT out now, let's close this.

@obenland obenland closed this May 10, 2021
@simison simison deleted the update/spt-rendering-template-item branch May 11, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Goal] Page Templates [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Status] Stale [Type] Enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants