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

Editor: Update WordPress packages to use with WordPress 5.8 #1176

Closed
wants to merge 6 commits into from
Closed

Editor: Update WordPress packages to use with WordPress 5.8 #1176

wants to merge 6 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 7, 2021

Trac ticket: https://core.trac.wordpress.org/ticket/52991

I executed npm run wp-packages-update and npm run build:dev to update the block editor to the latest version based on WordPress packages published to npm.

It works!!!

Screen Shot 2021-04-07 at 14 35 23


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@gziolo gziolo added the dependencies Pull requests that update a dependency file label Apr 7, 2021
@gziolo gziolo self-assigned this Apr 7, 2021
@gziolo gziolo requested a review from youknowriad April 7, 2021 12:33
@gziolo
Copy link
Member Author

gziolo commented Apr 7, 2021

I’m surprised to see it happening, but it looks like the update for WordPress packages works on the first try in core. The version of the Gutenberg plugin (v10.3.1) is ready to land.
It would be great to learn which PHP changes are missing and need to be backported, but I think we can do it separately. I haven’t tried enabling FSE features or installing the Gutenberg plugin.

@youknowriad
Copy link
Contributor

I think we should compare the "lib" folder between the two Gutenberg versions used previously and the new one.

@youknowriad
Copy link
Contributor

Do you know which version was used before?

@youknowriad
Copy link
Contributor

I think for instance, that there are a number of changes to the block-supports folder (all of them maybe?) that should be backported.

@gziolo
Copy link
Member Author

gziolo commented Apr 7, 2021

Do you know which version was used before?

Not yet, but I'm going to check it soon.

There are some changes in how blocks are aligned when the plugin gets activated:

Screen Shot 2021-04-07 at 14 43 19

The most important part is that it works with the plugin installed so it feels like we can merge those changes as the first step and backport all the missing bits afterward. What do you think?

@youknowriad
Copy link
Contributor

The most important part is that it works with the plugin installed so it feels like we can merge those changes as the first step and backport all the missing bits afterward. What do you think?

I don't know to be honest, I wouldn't want to merge a broken experience personally, at least we should identify them and have tickets for them. I'm going through some of theme right now WordPress/gutenberg@v9.9.3...v10.3.2

For example, I see the lazy loading related changes as well.

@gziolo
Copy link
Member Author

gziolo commented Apr 7, 2021

Many of the changes are behind the feature flag for FSE themes on PHP side. I'm not sure how much of the code is also removed from the JS build for those features marked as experimental which is most of the FSE blocks.

@gziolo gziolo requested a review from draganescu April 7, 2021 12:50
@youknowriad
Copy link
Contributor

youknowriad commented Apr 7, 2021

Here's what I got so far, I only kept the changes that should probably be backported (not experimental things)

  • Block Supports changes (even if it uses experimental flags, these flags are used in Core blocks, so this is mandatory)
  • Inline lazy loading (I don't know if this impacts the experience if not backported, maybe it's optional and should have its own ticket) Independent
  • New style handles to register and tweak dependencies (classic.css and reset.css, reusable-blocks.css). this is very important
  • Prevent the default editor styles from being loaded. this is very important too
  • Update Reusable block CPT labels
  • editor settings function (you know better for this one)
  • WP_REST_URL_Details_Controller (not sure about that one and whether it's used by stable JS things)
  • Layout block support and « supportsLayout » editor setting. (this might require some theme.json related stuff so we could potentially skip until we backport theme.json stuff)
  • New block category (theme) though maybe it’s just for FSE blocks now (need to confirm that done in https://core.trac.wordpress.org/changeset/50564)
  • Add object-position to the allowed kses attributes

@gziolo
Copy link
Member Author

gziolo commented Apr 7, 2021

Thank you @youknowriad for checking. I converted your comment into todo list and marked the last item as done. I took care of it 2 weeks ago.

@gziolo
Copy link
Member Author

gziolo commented Apr 15, 2021

Inline lazy loading (I don't know if this impacts the experience if not backported, maybe it's optional and should have its own ticket)

@aristath, can you help us decide whether it's mandatory to include for WordPress 5.8 or more importantly in this context if that has any relation to JS code?

@draganescu
Copy link

WP_REST_URL_Details_Controller arrived via #18042 and would be used by #19387 which is not ready yet.

@aristath
Copy link
Member

aristath commented Apr 15, 2021

@aristath, can you help us decide whether it's mandatory to include for WordPress 5.8 or more importantly in this context if that has any relation to JS code?

inline lazy styles loading has no JS dependencies so it can go in 5.8 👍
It is completely opt-in (by default enabled for FSE but otherwise opt-in is required) and won't break anything out of the box, so it would be great to have it in core and allow developers to start taking advantage of it.

@youknowriad
Copy link
Contributor

@aristath I'm reading that it can go into its own dedicated ticket right? And updating the packages won't break anything if it's not done at the same time?

@aristath
Copy link
Member

@aristath I'm reading that it can go into its own dedicated ticket right? And updating the packages won't break anything if it's not done at the same time?

Yes, updating the packages won't break anything if not done at the same time. It can be a separate, dedicated ticket.

@@ -0,0 +1,40 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

This one is only half backported, because it's only used for themes with theme.json which is not in Core yet. So for now I only included the mandatory filter to make themes without theme.json work properly.

@youknowriad
Copy link
Contributor

Did another review of the diff and checked a few items on the list, I think we're good.

We're missing two things:

  • the editor settings refactoring (this can potentially be separate as I don't think it impacts frontend, it's just a refactoring)
  • lazy loading (can we create a ticket and link to it as a reference)

There are three things that we should remember to do when back porting theme.json changes (if they land on 5.8)

  • Fully backport the layout block support,
  • Add a conditional check about the "classic.css" dependency in the style loader (only load it for non theme.json themes),
  • Add the supportsLayout editor setting.

@gziolo
Copy link
Member Author

gziolo commented Apr 15, 2021

I will fix the issues reported by CI, improve PHPDoc comments and test locally.

We're missing two things:

  • the editor settings refactoring (this can potentially be separate as I don't think it impacts frontend, it's just a refactoring)
  • lazy loading (can we create a ticket and link to it as a reference)

Right, those can land separately 👍🏻

We also need to add unit tests for newly added functionality in the follow-up patches

  • block supports
    • align
    • border
    • colors
    • custom classname
    • generated classname
    • layout
    • padding
    • typography
  • block_has_support

The best news is that everything looks like in the Gutenberg plugin after all changes applied by @youknowriad 🎉

nylen pushed a commit to nylen/wordpress-develop-svn that referenced this pull request Apr 15, 2021
In the response to the discussion during the Dev Chat, I'm doing a first pass to keep WordPress packages up to date in the WordPress 5.8 release cycle.

See WordPress/wordpress-develop#1176 for more details.

Props youknowriad, aristath, andraganescu.
See #52991.



git-svn-id: https://develop.svn.wordpress.org/trunk@50761 602fd350-edb4-49c9-b593-d223f7449a82
pento pushed a commit that referenced this pull request Apr 15, 2021
In the response to the discussion during the Dev Chat, I'm doing a first pass to keep WordPress packages up to date in the WordPress 5.8 release cycle.

See #1176 for more details.

Props youknowriad, aristath, andraganescu.
See #52991.



git-svn-id: https://develop.svn.wordpress.org/trunk@50761 602fd350-edb4-49c9-b593-d223f7449a82
@gziolo
Copy link
Member Author

gziolo commented Apr 15, 2021

All changes committed in https://core.trac.wordpress.org/changeset/50761.

@gziolo gziolo closed this Apr 15, 2021
@gziolo gziolo deleted the update/wordpress-dependencies-5.8 branch April 15, 2021 14:42
@youknowriad
Copy link
Contributor

Would love if we can run the e2e tests against trunk. I tried running the command but it's failing because of wp-env somehow.

markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Apr 15, 2021
In the response to the discussion during the Dev Chat, I'm doing a first pass to keep WordPress packages up to date in the WordPress 5.8 release cycle.

See WordPress/wordpress-develop#1176 for more details.

Props youknowriad, aristath, andraganescu.
See #52991.


Built from https://develop.svn.wordpress.org/trunk@50761


git-svn-id: http://core.svn.wordpress.org/trunk@50370 1a063a9b-81f0-0310-95a4-ce76da25c4cd
gMagicScott pushed a commit to gMagicScott/core.wordpress-mirror that referenced this pull request Apr 15, 2021
In the response to the discussion during the Dev Chat, I'm doing a first pass to keep WordPress packages up to date in the WordPress 5.8 release cycle.

See WordPress/wordpress-develop#1176 for more details.

Props youknowriad, aristath, andraganescu.
See #52991.


Built from https://develop.svn.wordpress.org/trunk@50761


git-svn-id: https://core.svn.wordpress.org/trunk@50370 1a063a9b-81f0-0310-95a4-ce76da25c4cd
@peterwilsoncc
Copy link
Contributor

Thanks for following this up folks. It looks like my instinct was correct that it would require more than a simple package update so I'm glad I left it for the experts. :)

@gziolo
Copy link
Member Author

gziolo commented Apr 16, 2021

@peterwilsoncc, actually I also backported changes from 5.7.1 in a45c871 as a temporary step. However, @youknowriad helped to identify missing PHP changes and here we are enjoying all the latest stable features of Gutenberg in WordPress trunk 😄

F-Wilke pushed a commit to FiliagoDev/WordPress that referenced this pull request Jul 31, 2021
In the response to the discussion during the Dev Chat, I'm doing a first pass to keep WordPress packages up to date in the WordPress 5.8 release cycle.

See WordPress/wordpress-develop#1176 for more details.

Props youknowriad, aristath, andraganescu.
See #52991.


Built from https://develop.svn.wordpress.org/trunk@50761


git-svn-id: http://core.svn.wordpress.org/trunk@50370 1a063a9b-81f0-0310-95a4-ce76da25c4cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
5 participants