(5P) Starter Page Templates: Fetch assets from template before insertion

[marekhrabe]

Changes proposed in this Pull Request

New flow after selecting a template:

• analyze blocks used and look for images in them:
  • if there are none, proceed right away or after a short delay (that would address (3P) Add an artificial loading delay before displaying Editor upon selecting Template #34570)
  • if there are any, call API that fetches images from remote URLs into the site and returns their new URLs and attachment ids. Replace old URLs and ids with those local ones
• (optional) loading state
• insert template into the editor and close modal

TODO:

• add support for core/cover
• core/gallery blocks
• core/media-text blocks
• document functions, params, return values…
• gracefully handle URLs with resizing arguments (like ?w=640)
• replace mocked API with real one
• introduce a "loading state" between template selection and showing it inside editor

Testing instructions

Install plugin on your WP, following https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/README.md

• Create a new page and use the tempalte selector
• About or Contact templates should be inserted immediately (but the modal itself has a 300ms delay before it closes)
• Other templates using images should fire a request to import images (follow your devtools and look for POST /wp-json/fse/v1/batch)
• After the request succeeds, template is inserted into the editor and modal closes 300ms after
• Inspect images inside the block editor - they should all be loaded from the same domain.
• Check media library for new images.

Fixes #34603
Fixes #34570

[marekhrabe]

The animated WP logo outline which might be a possible loading state comes from here: https://github.com/WordPress/gutenberg/blob/bb8debab915e9fab373e24f9e9dd9469ac27779a/packages/editor/src/components/post-preview-button/index.js#L19

It's currently embedded with the preview button itself (not sure if good idea) so we can either make some sort of abstraction or copy/paste the svg asset + animation 🤷‍♂️

[matticbot]

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.

[obenland]

9ae3312 makes use of the API endpoint. There are a few things worth discussing and adding to our todo list:

• Currently the API doesn't care if images already exist and sideloads everything. It might be worth adding a mechanism to not do that. Maybe by saving the original URL in post meta?
• The loading delay is significant. We should see if we can run that request async, after the content has been inserted, if possible
• The batch API Endpoint is very generic atm. Maybe that's not needed?

[marekhrabe]

The loading delay is significant. We should see if we can run that request async, after the content has been inserted, if possible

I'm affright this might not be possible - that's why I've started this by delaying the insertion. Once the blocks are inserted, any other change will create an entry in the todo undo stack. Allowing users to undo image replacements and also if they are quick enough to click edit on image/gallery they will end up in a broken state :(

Currently the API doesn't care if images already exist and sideloads everything. It might be worth adding a mechanism to not do that. Maybe by saving the original URL in post meta?

I have mentioned using postmeta for this in the other PR. Sounds like a good optimisation to do. You should see my media library on the site I test this PR with :D

The batch API Endpoint is very generic atm. Maybe that's not needed?

Maybe we can simplify it to just accept image URLs? It could route requests on its own to the right place.

[marekhrabe]

I've build a support for a loading state but I haven't done anything in the UI. Let's do it as a separate task. State already has all the info required so minimal code to do it would be something like this:

<div>
  {
    this.state.isLoading
    ? <p>We are loading template { this.state.selectedTemplateTitle }.</p>
    : // …render TemplateSelectorControl here…
  }
</div>

(Written to be somewhere in the render() of starter-page-templates/page-template-modal/index.js)

[marekhrabe]

Regarding the time it takes to import images - let's make sure we optimize images in our templates - jpg, appropriate resolution and compression. The timing might be a pain point at this moment as inserting the template is the only way to preview it but if we make better previews in-modal, I think it wouldn't be that big of an issue.

[obenland]

The loading delay is significant. We should see if we can run that request async, after the content has been inserted, if possible

I'm affright this might not be possible - that's why I've started this by delaying the insertion. Once the blocks are inserted, any other change will create an entry in the todo undo stack. Allowing users to undo image replacements and also if they are quick enough to click edit on image/gallery they will end up in a broken state :(

@youknowriad @mtias Is there any way you can think of where we could replace images after the template has been inserted, without adding to the todo undo stack?

[mtias]

Is there any way you can think of where we could replace images after the template has been inserted, without adding to the todo undo stack?

See WordPress/gutenberg#8119

[obenland]

I just tested it with D31446-code to use the optimized and smaller images, and it's still an excruciatingly long wait time after selecting a template. For the portfolio template the sideloading request alone took 10.46s.

I don't know if the work on large previews could help us soften that blow somehow, but for now it just simply takes too long :(

[marekhrabe]

Have you tested on the sandbox or locally? I just did some more testing on my sandbox and noticed bugs that I'll need to address.

The main one is that on wpcom all requests go through the common API proxy and somehow we have double slash there which prevents it from working. Example URL: https://public-api.wordpress.com/wp/v2/sites/112010505//fse/v1/sideload/image/batch?_envelope=1&_locale=user

[obenland]

I created D31603 as a fix for the API endpoint on dotcom

[retrofox]

I've tested the current approach and it seems to work properly according to the description.

It hits the /batch endpoint:

The template images are loaded from the local server:
http://localhost/wp-content/uploads/2019/08/36ba8-adventure-calm-clouds-414171.jpg

And the image is in the media gallery.

The problem here now is we've changed how the template selector works. Although it doesn't seem to be a big deal, we will need to rebase with master and adapt the behavior of this PR. The main difference is when the user selects the template. Now, instead of injecting it into the Editor we are showing the preview.

[marekhrabe]

Although it doesn't seem to be a big deal, we will need to rebase with master and adapt the behavior of this PR

I started on Friday and have a WIP locally. Still a ton to update, the changes are significant…

[retrofox]

I started on Friday and have a WIP locally. Still a ton to update, the changes are significant…

Yeah, that's true.
Also, maybe we could take advantage of these changes and consider a similar approach keeping in mind that now we are showing all templates through of the thumbnails.
Thinking this as an opportunity to move the images to the local server before to insert the template into the Editor.

[obenland]

Anytime I come across this, I wonder if it may be time to separate it out into a function.

[marekhrabe]

This feature is now controlled through this prop: https://github.com/Automattic/wp-calypso/pull/34839/files#diff-a664d83ace51cb87304bf656c9e9f303R244

disabled by default so we can merge the code and evolve it while keeping the old behavior