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 published for Gutenberg 10.6 #1257

Closed
wants to merge 7 commits into from
Closed

Editor: Update WordPress packages published for Gutenberg 10.6 #1257

wants to merge 7 commits into from

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented May 14, 2021

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

TODO

It needs more work:

  • All newly exposed core blocks aren't registered on the server. It requires changes in webpack build and some references for PHP files for most of the blocks.
  • Duotone supports need to be backported (Add duotone block supports gutenberg#26752).
  • Other PHP changes between 10.5 and 10.6 need to be validated.
  • Changes to lib/block-patterns/three-columns-media-text.php pattern – it looks up to date
  • Changes to layout and border block supports
  • Some php changes to support the iframed template editor cc @ellatrix – it's tracked separately and it depends on other changes

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 self-assigned this May 14, 2021
@@ -1003,7 +1003,7 @@ module.exports = function(grunt) {
WORKING_DIR + 'wp-{admin,includes}/**/*.js',
WORKING_DIR + 'wp-content/themes/twenty*/**/*.js',
'!' + WORKING_DIR + 'wp-content/themes/twenty*/node_modules/**/*.js',
'!' + WORKING_DIR + 'wp-includes/js/dist/vendor/*.js',
'!' + WORKING_DIR + 'wp-includes/js/dist/**/*.js',
Copy link
Member Author

Choose a reason for hiding this comment

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

We no longer publish ES5 code to npm. All WordPress packages work with ES2015+ from now on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you clarify the change a bit more :p

Copy link
Member Author

Choose a reason for hiding this comment

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

This excludes WP packages from JSHint validation. js/dist folder contains files built from node_modules with webpack.

@gziolo gziolo requested a review from youknowriad May 14, 2021 13:17
@gziolo gziolo added the dependencies Pull requests that update a dependency file label May 14, 2021
@youknowriad
Copy link
Contributor

I updated the todo list with a few required items as well.

@gziolo
Copy link
Member Author

gziolo commented May 17, 2021

Site Logo block registers 3 filters in addition to block registration:
https://github.com/WordPress/gutenberg/blob/bed089677e91b4ae00916645139c7f806f46ce8b/packages/block-library/src/site-logo/index.php#L74-L75
https://github.com/WordPress/gutenberg/blob/bed089677e91b4ae00916645139c7f806f46ce8b/packages/block-library/src/site-logo/index.php#L126

Do you think we should apply those changes directly to WP core?

Those changes fail 2 unit tests:

1) Tests_General_Template::test_has_custom_logo
Failed asserting that true is false.

/var/www/tests/phpunit/tests/general/template.php:265

2) Tests_General_Template::test_get_custom_logo
Failed asserting that a string is empty.

/var/www/tests/phpunit/tests/general/template.php:310

I think those filters will get executed twice once we land the block in WP core.

@@ -0,0 +1,392 @@
<?php
/**
* Duotone block support flag.
Copy link
Member Author

@gziolo gziolo May 17, 2021

Choose a reason for hiding this comment

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

As discussed in WordPress/gutenberg#26752 (comment) with @ajlende, we need to include the license detailed for the code derived from the TinyColor library.

@gziolo
Copy link
Member Author

gziolo commented May 18, 2021

I scanned quickly changes applied between 10.5 and 10.6 and I might have covered all of them with the last commit. We need to double-check: WordPress/gutenberg@release/10.5...release/10.6.

I backported all changes for block supports in 934ec7a. I left there 2 TODO items where I had to removed logic that depends on Global Styles in the layout support. It's something we need to explicitly list for the next round so it's wired correctly.


$used_layout = $block['attrs']['layout'];
if ( isset( $used_layout['inherit'] ) && $used_layout['inherit'] ) {
// TODO: Add theme.json handling for the default layout.
Copy link
Member Author

Choose a reason for hiding this comment

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

@youknowriad, it might be that you didn't backport this part earlier because it makes sense only when Global Styles are in place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I left the whole "layout" support for later (when theme.json lands) because it should be disabled for themes without that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, so we probably can remove it here as well. It's most likely dead code anyway.

@gziolo
Copy link
Member Author

gziolo commented May 18, 2021

I found the logic that is related to:

Some php changes to support the iframed template editor

https://github.com/WordPress/gutenberg/blob/b5f6955742e784f11828e1dacba4361713135963/lib/client-assets.php#L707-L744

It looks like something that could be done separately.

( true === _wp_array_get( $block_type->supports, array( '__experimentalBorder' ), $default ) )
) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This part can probably use block_has_support as well without the nested $feature thing but I guess that comment is better in the Gutenberg repo :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, let's fix in Gutenberg and backport with 10.7 version.

@youknowriad
Copy link
Contributor

@gziolo did we include the 10.6.1 package update and php changes here?

@gziolo
Copy link
Member Author

gziolo commented May 18, 2021

did we include the 10.6.1 package update and php changes here?

If 10.6.1 was published this week, then the answer is no.

@youknowriad
Copy link
Contributor

I noticed that the editor styles are not applied properly (compared to the plugin), not sure why. You can compare with tt1 theme, you'll see that the background is not applied

@youknowriad
Copy link
Contributor

Nevermind, I was testing with tt1-blocks and theme.json based styles are not backported yet, so we're all good here.

@ellatrix
Copy link
Member

Looking at the iframe stuff. Trying to get this branch running first.

@gziolo gziolo marked this pull request as ready for review May 18, 2021 14:18
@gziolo
Copy link
Member Author

gziolo commented May 18, 2021

I removed the Site Logo block for now because it creates too much friction, and I couldn't find a simple way to make failing tests pass without changing assertions. It needs to be investigated separately.

Technically speaking, this block will be still exposed in the inserter, but it won't work correctly until we include PHP bits.

@gziolo
Copy link
Member Author

gziolo commented May 19, 2021

@gziolo gziolo closed this May 19, 2021
@gziolo gziolo deleted the update/gutenberg-plugin-10.6 branch May 19, 2021 16:29
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
Development

Successfully merging this pull request may close these issues.

3 participants