-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Stabilize the __experimentalBorder support #65968
Stabilize the __experimentalBorder support #65968
Conversation
… the non __experimental prefix
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @benazeer-ben! In case you missed it, we'd love to have you join us in our Slack community. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
Thanks so much for working on this @benazeer-ben! There are a few WordPress coding standard issues that are flagged here, but otherwise, this is looking very good. @Mamaduka or @gziolo would you be able to do a more comprehensive code review? I just tested it with my own Icon Block, which is currently using With
|
Editor | Front end |
---|---|
With border
Editor | Front end |
---|---|
I do want to note that as a follow-up to this PR, we will need to update all Core blocks that currently use |
Hi @ndiego We are waiting for the code review feedback and accordingly proceed adding support for other blocks. |
Sorry for the delay. WordPress 6.7 is taking much of the focus right now. @andrewserong forgive the ping, but perhaps you might be able to do a code review whenever you have a sec? Everything seems to work correctly in my testing. |
I also want to note that additional PRs will be needed to complete the task list in the original issue, or you could bundle everything here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @ndiego and for working on this @benazeer-ben! I'm a little short on time this week, so I've only given this a quick pass but will hopefully take it for a more detailed spin next week.
I've left a couple of small comments. Something I was wondering about is whether the PHP implementation should also perform a transformation? I tried that in #63401 for the Typography supports, but it did also add a little extra complexity.
Overall, I like that you've gone with the simplest possible change for this PR, that seems like a pragmatic path forwards. Having the Border support (in PHP) be aware of both the experimental and non experimental properties sounds reasonable to me 👍
Once this is ready for review, similar to #63401, I think it'd be good to do a wider ping / gather more folks to help testing and confidence check the approach. I'm a little biased as I already think this is a good way to do it, so I am a tiny bit cautious about approving on my own 🙂
lib/block-supports/border.php
Outdated
@@ -17,7 +17,7 @@ function gutenberg_register_border_support( $block_type ) { | |||
$block_type->attributes = array(); | |||
} | |||
|
|||
if ( block_has_support( $block_type, array( '__experimentalBorder' ) ) && ! array_key_exists( 'style', $block_type->attributes ) ) { | |||
if ( block_has_support( $block_type, array( 'border', '__experimentalBorder' ) ) && ! array_key_exists( 'style', $block_type->attributes ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tested this, but I think this needs to have two block_has_supports
checks (one for border
and one for __experimentalBorder
) as an array passed to this function is treated as a path, rather than an OR. I see there's a similar check down in line 158 that looks correct to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your suggestions @andrewserong
I have updated this case in the recent commit.
lib/block-supports/border.php
Outdated
'width' => isset( $border['width'] ) && ! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'width' ) ? $border['width'] : null, | ||
'color' => isset( $border['color'] ) && ! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'color' ) ? $border['color'] : null, | ||
'style' => isset( $border['style'] ) && ! wp_should_skip_block_supports_serialization( $block_type, '__experimentalBorder', 'style' ) ? $border['style'] : null, | ||
'width' => isset( $border['width'] ) && ! wp_should_skip_block_supports_serialization( $block_type, 'border', 'width' ) ? $border['width'] : null, | ||
'color' => isset( $border['color'] ) && ! wp_should_skip_block_supports_serialization( $block_type, 'border', 'color' ) ? $border['color'] : null, | ||
'style' => isset( $border['style'] ) && ! wp_should_skip_block_supports_serialization( $block_type, 'border', 'style' ) ? $border['style'] : null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to handle the skip serialization for the __experimentalBorder
case, too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order to make sure that, now I have added the skip serialization for the __experimentalBorder case too.
@@ -61,7 +61,22 @@ function mergeBlockVariations( | |||
|
|||
return result; | |||
} | |||
function handleExperimentalBorder( rawSupports ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We'll also need to handle the transformation for Typography at some point (which I worked on in #63401). Would it be worth renaming this to stabilizeSupports
so that we have one function call, and we can add other stabilized supports to the body of the function in follow-ups?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Function name has been updated as you mentioned above.
…o iteration/stabilize-experimental-border-prefixes
Thank you for all your work and patience here @benazeer-ben 🙇 There have been a number of PRs attempting to stabilize the border block supports, each a little different from the last. Now that the typography block support stabilization in #63401 has landed we have a best practice guide on how to achieve this. An alternative PR (#64330) has mostly been on my radar but both that PR and this have some missing pieces in different ways. I've put up a fresh PR that extends the approach in #63401. I'd like to propose closing this PR in favour of #66918 so we can all consolidate around a single approach. How's that sound to you? I'd also like to stress that the work you've done here has been great and is appreciated! |
What?
Updating php code and JS implementation to work both '--experimental' and non '--experimental ' prefix.
#64312
Why?
This issue outlines the tasks needed to stabilize the __experimentalBorder support. Doing so will make it easier for third-party extenders to confidently build custom blocks that provide border support and modify existing blocks with this support.
How?
Updating PHP block support code in lib/block-supports/border.php to use the non __experimental prefix, falling back to the __experimental prefix if available.
Also updated JS implementation to use the non __experimental prefix.
Testing Instructions
Open a post or page.
Apply border to any of the section in the post/ page/ template.
Border functionality will work fine even after updating the code with non experimental prefixes.
Note: Here we need to update the WP core blocks' block.json files to use the non __experimental prefixes.