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

Editing Toolkit: Load patterns from the rest API endpoint #45926

Merged
merged 12 commits into from
Oct 8, 2020

Conversation

apeatling
Copy link
Member

@apeatling apeatling commented Sep 24, 2020

Remove block patterns from within the codebase, and load them via the new rest API endpoint.

Testing instructions

  • Load the editor and the patterns inserter and confirm you see all the following categories:

Screen Shot 2020-10-08 at 12 50 05 PM

  • Try inserting a pattern and confirm this works as expected.
  • Switch your blog to a different language (settings->general)
  • Confirm that you can still see patterns in the pattern inserter
  • Some patterns may be translated (still confirming this).

@apeatling apeatling added [Feature] Block Patterns Pattern content itself, and the functionality that lets you create patterns. Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin labels Sep 24, 2020
@apeatling apeatling self-assigned this Sep 24, 2020
@matticbot
Copy link
Contributor

@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 Sep 24, 2020
@matticbot
Copy link
Contributor

Caution: This PR affects files in the Editing Toolkit Plugin on WordPress.com
Please ensure your changes work on WordPress.com before merging.

D50169-code has been created so you can easily test it on your sandbox. See this FieldGuide page about developing the Editing Toolkit Plugin for more info: PCYsg-ly5-p2

@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.

@glendaviesnz
Copy link
Contributor

This worked well for me ... I didn't see the 'Seedlet' category at the top of the list though, was this category removed?

Also, what is the plan with the pattern transient cache invalidation? Currently it looks like it will only be invalidated when the editing toolkit plugin is updated, but do we want a way of triggering an update without have to publish a new version of the plugin? eg. would it be better to trigger it via detection of some update of data on dotcompatterns.wordpress.com?

Copy link
Member

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

This is working pretty well for me in testing, thanks for putting it together so quickly! I left a few comments of small things to change. The main things were that the .org patterns are currently being shown and they weren't previously (wasn't sure if that's intended), and we probably want to add the 'categories' => 'pattern' param to the API call to hide the full page layouts (unless they're intended to be shown, too). Also one of the contact patterns is showing up in the Uncategorized category, so this one might need to be fixed up on the source site.

Nice idea pulling in transients so that we don't have to make the API call on every request, that seems like a solid approach, and I like that updating the plugin or a time period being elapsed will clear the cache. My only concern here is ensuring that we can easily invalidate the cache if we need to, I'm too sure what the risk is there, though. Would setting the expiration time for the transient to a shorter period help, say 6 or 12 hours instead of a full day?

Also, this is my first time testing out the pattern categories drop-down — nice addition! Makes the browser performance of the pattern inserter so much nicer.

The final concern I have is just the same one about doing so much on the initial page load. I think this is mostly mitigated by using transients here, and hopefully adding object cache at the endpoint will help us for speeding up getting the translated patterns. Before this PR is merged, I think it'd be good to do a check on initial editor load time difference between before and after the PR, just so that we can have a clear idea of the potential performance impact. Longer-term, 🤞 we can move to loading patterns via JS in the editor.

@apeatling
Copy link
Member Author

The main things were that the .org patterns are currently being shown and they weren't previously (wasn't sure if that's intended)

I wasn't sure about hiding dotorg patterns, this seemed odd to me. @ianstewart could you chime in here and let me know if you want that to continue?

@ianstewart
Copy link
Contributor

We previously had, I think, an inconsistent subset of them hidden. Some showed, some didn't. It was an intentional choice, partly because they weren't translated, partly because I think they were just a placeholder set. I'd probably keep hiding them for the former reason if they won't be automatically translated.

@apeatling
Copy link
Member Author

This worked well for me ... I didn't see the 'Seedlet' category at the top of the list though, was this category removed?

@glendaviesnz This will only show with Seedlet theme activated. I'm seeing the category, can you confirm you can't see it with that theme active?

@apeatling
Copy link
Member Author

Thanks for the reviews. I've fixed up everything I can on this PR, once I get some more feedback on the conversations above I'll fix up the rest. 👍

@glendaviesnz
Copy link
Contributor

@glendaviesnz This will only show with Seedlet theme activated. I'm seeing the category, can you confirm you can't see it with that theme active?

🤦 Doh, that makes sense, yeh, shows for me once the Seedlet theme is activated.

@glendaviesnz
Copy link
Contributor

This tests well for me still after latest changes.

@apeatling
Copy link
Member Author

The updates have been made to support the new API changes. I think this is ready for a final review.

@andrewserong
Copy link
Member

The code's looking good to me now @apeatling, thanks for following up with all the changes! Tested manually on a simple site, and I haven't been able to detect a performance impact with the change, which is great.

I noticed one pattern appeared to render a block validation error in the preview in the inserter, as well as when I add it, but I'm not quite sure what's going on there. It's one of the "Call to Action" patterns (id 1638):

image

I might need to do some more digging on Monday, as it's opening fine for me in the source site's wp-admin. 🤔

Lastly, because we're still running GB 9.0.0 on simple sites, we don't have the block pattern category drop down you put together, so I haven't really been able to test the patterns down the bottom of the inserter list. I think we should wait until GB 9.1.0 lands in simple sites before merging this one. I'm happy to do another round of testing once the upgrade has gone through.

@apeatling
Copy link
Member Author

Sounds good, happy to wait for 9.1, I've posted a happiness heads up. For the block validation issue, I haven't seen that. Do you have coblocks installed? Some of the patterns need it.

@andrewserong andrewserong force-pushed the add/patterns-toolkit-rest-api-support branch from d688e78 to f305656 Compare October 5, 2020 06:28
@andrewserong
Copy link
Member

Thanks Andy! This was on a simple site, so the Dotcom-flavour of coBlocks should be there. I was having trouble applying the diff in my sandbox today (I think possibly because there's been a backend deployment of the editing toolkit plugin since this PR was opened?) so I've given the PR a rebase. I'll do more testing when we've got GB v9.1 in place :)

@apeatling
Copy link
Member Author

@andrewserong I've done some extensive testing on my sandbox and things look good. I think this is ready to go.

@apeatling
Copy link
Member Author

Going to merge this to catch the 2.7 editing toolkit release.

@apeatling apeatling merged commit 940db60 into master Oct 8, 2020
@apeatling apeatling deleted the add/patterns-toolkit-rest-api-support branch October 8, 2020 18:54
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 8, 2020
@apeatling
Copy link
Member Author

Reverting this PR because when the API is called needs to be more specific (on the editor screen only). Also need to check on endpoint caching.

apeatling added a commit that referenced this pull request Oct 8, 2020
@apeatling apeatling restored the add/patterns-toolkit-rest-api-support branch October 8, 2020 21:28
@apeatling
Copy link
Member Author

Will open a new PR with fixes to make the API call more specific.

apeatling added a commit that referenced this pull request Oct 8, 2020
robertf4 pushed a commit that referenced this pull request Oct 9, 2020
* Load patterns from the source site via the patterns toolkit rest endpoint. Remove patterns code in the plugin.

* Fix site ID data type

* Actually fix the right var.

* No need to specific the source site, removing this.

* Restore the removal of core patterns and columns category.

* Change API calls to match new API format.

* Line up => for phpcs.

* Use spaces to line up.

* Remove the two columns of text core pattern.

* Remove incorrect comment.

* Order the pattern categories alphabetically.

* Fix phpcs errors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Editing Toolkit For issues and PRs that affect the Editing Toolkit plugin [Feature] Block Patterns Pattern content itself, and the functionality that lets you create patterns.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants