Borders: Stabilize border block supports within block processing #66918
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +89 B (0%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
Quick note: I need to double-check and test a few more edge cases on this before it will be ready for review. I'll flag accordingly when it is. |
|
Update:
The stabilization of the |
|
The issue noted above seems to have been isolated to how the global styles unit test set up the fake image block. Fixed in eb75855. I've run through the test instructions in the PR description a couple of times in different ways. It's behaving pretty well for me. Fresh eyes might be able to see some edge cases I've missed though. I'll flag this ready for review now. Given the backport will take a slightly different approach, I'll follow up with a backport once I'm sure the approach here is stable. A single backport might also cover both the typography stabilization and this. Note: The failing e2es are unrelated and should be temporary. I'll kick them off again in due course. |
aaronrobertshaw
requested review from
tellthemachines,
spacedmonkey and
ellatrix
as code owners
Ah, so in that case because it's passing |
|
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. |
aaronrobertshaw
force-pushed
the
update/stabilize-border-block-supports
branch
from
eb75855
to
f51cbc0
Compare
That's correct. I'm open to ideas on better approaches. To me, the test here's aim was to cover the happy path of selector generation. The unit tests around stabilizing block supports cover ensuring that happens correctly. |
|
Regarding the backport for this PR, after discussion with @andrewserong, the plan is to create a single backport for the stabilization of all block supports. That will happen via updates to the existing backport for typography supports in WordPress/wordpress-develop#7069. Some of the reasoning for this approach includes:
|
There was a problem hiding this comment.
Nice work here, I haven't been able to fault it! 🎉
✅ The code changes look good to me, I like how it generalises the stabilization to include both typography and border, as well as creating a good structure for subsequent stabilization
✅ Thanks for improving the logic of the stabilization so that it factors in the order in which keys were added 👍
✅ Works well as-is for experimental border and typography
✅ With the snippets applied, the stabilized keys appear to work well for static and dynamic blocks, in the post editor and in global styles
✅ Updating the image (and other) blocks to use the stabilized Border key works as expected in global styles
LGTM! 🚀
We've chatted a bit about this work, so I've already got a fair amount of context for what's happening here. It might be worth getting a second review and/or opinion (e.g. possibly @ndiego) to confidence check as I'm potentially biased toward this approach, but I'm really liking this PR, along with the code quality improvements it adds!
| $experimental_first = $experimental_index < $stabilized_index; | ||
|
|
||
| // Update support config, prefer the last defined value. | ||
| if ( is_array( $config ) ) { |
There was a problem hiding this comment.
This is not at all a blocker for now, but in the JS implementation we check for an object, so there'd never be a match against the align support, where an array is a valid value. Here, there theoretically could be a match.
In practice it's not an issue because we don't have anything in $experimental_to_stable_keys that references supports where an array is a valid value, but just thought I'd mention it for the (very) unlikely event that at some distant point in the future we create an experimental support that does accept an array and then want to stabilize it further down the track. In that case, this condition might need to be updated.
For now, though, I think it's fine to leave as-is. And please excuse my verbose comment here 😄
There was a problem hiding this comment.
Appreciate you raising this one. It has vaguely been on my radar but I was struggling to come up with a real-word case where it should happen.
The align support is stable, so wouldn't be in the $experimental_to_stable_keys array, as you note. In that case, the guard clause at the beginning of the loop should just continue on, while using the align config as is.
In the end, rather than waste too much time on coming up with hypotheticals to test, I decided to leave this until it was a problem.
Does that allay your concern at all? What else am I missing?
There was a problem hiding this comment.
I meant to also add that in recent times, there's been a decided effort to not land block supports at all unless they were stable. To prevent being in the situation like we are with some typography and border supports. That's been the case for entire block support features like shadow or subfeatures like textColumns and minHeight.
If the approach to not landing experimental block supports continues, there shouldn't be a scenario with a new experimental block support that needs stabilizing.
There was a problem hiding this comment.
Absolutely, what you've described captures my thinking, too, so you're not missing anything here!
In the end, rather than waste too much time on coming up with hypotheticals to test, I decided to leave this until it was a problem.
That's where I was coming from, too. I was mostly thinking out loud, which is to say, flagging something that could theoretically be an issue at some distant point in the future. And it's at that distant point (if we're adding an experimental support that uses arrays) where I think it could be addressed, rather than now 🙂
There was a problem hiding this comment.
If the approach to not landing experimental block supports continues, there shouldn't be a scenario with a new experimental block support that needs stabilizing.
Indeed — my vote is to avoid using the __experimental prefix for things in the future, too 👍
There was a problem hiding this comment.
👍 Having it on the record here will also flag it for our future selves or others, if it is really needed in the end.
|
This PR should be ready for a final second opinion now. There's a second PR based off this branch that shows this stabilization approach can also work for the shared experimental support flags, The logic in the stabilization function changes significantly in the follow-up PR however I think there is a benefit to some focused testing for each iteration of stabilization. That and there are already a lot of files being touched here 🤷 |
|
@karthick-murugan, @hbhalodia, and @benazeer-ben given your experiences taking different approaches to stabilizing block supports, it would be great if you could help test this PR and #67018 to ensure it's all working as expected. I'm pretty confident this is the direction we wish to go. If there are no major issues, we can address any further edge cases in follow-ups so we can start getting this tested in the wild via the next Gutenberg release. |
There was a problem hiding this comment.
Not having much context on this, but I see @andrewserong has previously tests.
I went through all testing steps in TT5, and TT2 because I was feeling sentimental.
All the border supports work as expected with both the stabile and __experimental prefixes. Default controls appear as expected, e.g., Group vs Site Title.
Setting __experimentalBorder to false via the filter disables the controls.
Nothing in the error logs or console I can see.
Nice job getting all the pieces together.
@aaronrobertshaw - I have tested this PR with all the testing instructions above and all works perfectly. I have attached the video reference of the testing. Also tested by updating the functions.php with the code snippet and it works perfectly. REC-20241120123327.mp4 |
|
Appreciate the review @karthick-murugan, thank you! 🙇 I'll give Ramon a chance to see if the updated inline comments improve clarity. Then I'll aim to merge this tomorrow and push forward #67018. |
Part of: #64312
Related:
What?
This PR aims to stabilize the border block support key, i.e.
__experimentalBorder-->border.Props to @hbhalodia and @benazeer-ben for their work to date in this area (see #64330 & #65968 respectively)
Why?
Border support has been around for a long time in its "experimental" form. Recent efforts to drive its adoption across core blocks highlight the need to make it stable. It's time.
Also, as the original issue notes:
How?
This PR builds upon the path to stabilizing block supports charted in #63401.
borderkey__experimentalBorderkeyprocessBlockTypefunction in the blocks store has been updated to stabilize the border key as per typography supportsTesting Instructions
Example block markup
The following snippet is a group block with background, border, and padding applied. It contains a paragraph and image block each with its own borders too.
Diff to update Group block to use stable border support key
Filter to test 3rd party use of old experimental key
function disable_group_borders( $args ) { if ( 'core/group' === $args['name'] ) { _wp_array_set( $args, array( 'supports', '__experimentalBorder' ), false ); } return $args; } add_filter( 'register_block_type_args', 'disable_group_borders', 20 );__experimentalBorderkey still display the border controls within the block inspector and Global Styles.__experimentalBordertoborder.borderkey. Confirm the global border styles for the Image block are still correct.Screenshots or screencast
There should be no visual changes. Any borders that don't match trunk are likely a regression.