Fix: Stabilize border block support keys #64330
Conversation
This commit references the PR - https://github.com/WordPress/wordpress-develop/pull/7069/files, which aims to stablise the typogrpahy supports. Based on the PR added this commit to stablise the supports for the border. Since we does not have the border options as experimental, instead we have border as experimiental hence we stablised the main key and this makes us not to touch the inner border supports as they were not defined as experimental.
There was a problem hiding this comment.
Thanks for your patience waiting on #63401 @hbhalodia 🙇
Now that's merged, I think we're ready to dust off this PR. If you are short on bandwidth, I'd be happy to push it forward.
the inner border supports as they were not defined as experimental.
To help provide clarity to others coming to this PR, the above appears to only reference the border-specific flags within the border supports property. Border support can still define keys prefixed with __experimental, e.g. __experimentalDefaultControls & __experimentalSelector.
Also, the main typography stabilization PR this one is referencing is #63401. The backport linked might need a refresh with the latest and hasn't merged yet, whereas the Gutenberg PR has been.
After reading through the PR in its current state, here's a preliminary list of next steps to move it forward, in case it helps.
- Move the stabilization of border supports via the
register_block_type_argsfilter to the 6.8 compat folder.- The stabilization here should also be rolled into the function added by the typography stabilization PR
- The logic also has a bug where it would completely replace existing config rather than merge.
- Roll the new JS function stabilizing border supports into the one introduced by the typography PR.
- This PR's function also has the same flaw as the PHP filter. It needs to merge the supports config not replace it entirely.
- Unit tests need updating, including:
packages/blocks/src/store/test/process-block-type.jsphpunit/block-supports/border-test.php
- Update the PR title and description
- This isn't a bug fix but rather an enhancement so perhaps drop
Fix:from the title - The description needs completing to provide context, test instructions etc to make it easier for folks reviewing.
- This isn't a bug fix but rather an enhancement so perhaps drop
As mentioned above, I'm happy to help out here whether that's pushing to this branch or spinning up an alternative PR. Let me know whatever works for you.
Thanks again for getting the ball rolling here.
| return $args; | ||
| } | ||
|
|
||
| $args['supports']['border'] = $args['supports']['__experimentalBorder']; |
There was a problem hiding this comment.
This would completely overwrite any prior config that already uses the stabilized key.
| return $args; | ||
| } | ||
|
|
||
| add_filter( 'register_block_type_args', 'gutenberg_stabilize_experimental_block_supports', PHP_INT_MAX, 1 ); |
There was a problem hiding this comment.
As this won't land until 6.8, we'll need to move this to the 6.8 compat folder. Given there's already the same filter equivalent for typography supports, we probably only need to move the border stabilization into that same filter function and rename.
I believe there is a small cost with filters, so while I like the separation of concerns to have each support in its own filter, there could be a benefit to have a single function to stabilize block supports.
| * | ||
| * @return {Object} Stabilized supports. | ||
| */ | ||
| function stabilizeBorderSupports( rawSupports ) { |
There was a problem hiding this comment.
The typography stabilization PR introduced a stabilizeSupports function. This border stabilization should probably be rolled into that too.
| supports?.__experimentalBorder && | ||
| typeof supports.__experimentalBorder === 'object' | ||
| ) { | ||
| supports.border = supports.__experimentalBorder; |
There was a problem hiding this comment.
This needs to merge with values already using the stabilized border property rather than flat out replacing it.
| @@ -102,13 +130,21 @@ export const processBlockType = | |||
| ), | |||
| }; | |||
|
|
|||
| blockType.supports = stabilizeBorderSupports( blockType.supports ); | |||
There was a problem hiding this comment.
We'll need some unit tests to cover all the stabilization, similar to those in #63401.
|
Hey @aaronrobertshaw, thanks for the feedback 🙇. I am currently a little short in bandwidth, sorry for that. I am okay with changes in the PR itself or spinning up a new one, whatever suits the best. Thank You, |
|
👍 Thanks for letting me know, @hbhalodia, not a problem at all. I appreciate your effort in getting the ball rolling here. I've been working on a separate branch as I found a few gaps and issues beyond just rebase conflicts. I'll spin up a separate PR soon and close this one accordingly. |
|
I'm closing this PR in favour of #66918 as discussed above. |
This commit references the PR - https://github.com/WordPress/wordpress-develop/pull/7069/files, which aims to stablise the typogrpahy supports. Based on the PR added this commit to stablise the supports for the border. Since we does not have the border options as experimental, instead we have border as experimiental hence we stablised the main key and this makes us not to touch the inner border supports as they were not defined as experimental.
What?
Issue - #64312
Why?
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast