[Mobile] - Spacer block - Add initial support for spacing presets #47258
Conversation
geriux
added
[Status] In Progress
|
Size Change: +101 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
geriux
added
the
Mobile App - Automation
There was a problem hiding this comment.
Thanks for the great walk through of the code and changes in the description. Very helpful!
From testing on iOS and Android with both the Twenty Twenty-Three and Masu themes, I noticed a couple things.
First, it appears the CSS unit parser fails to parse complex, nested CSS functions. E.g. the 70 preset from Twenty Twenty-Three is clamp(5rem, 5.25rem + ((1vw - 0.48rem) * 9.096), 8rem). The spacer appears to always fallback to the default 100px value. The nested parentheses seem to "trip up" the parser.
Updating the block syntax to use a simpler spacing preset resolves the issue. In the example below, the first block falls back to default value, the second block uses the spacing preset.
Example Spacer Blocks
<!-- wp:spacer {"height":"var:preset|spacing|70","style":{"spacing":{"margin":{"top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"}}}} -->
<div style="margin-top:var(--wp--preset--spacing--40);margin-bottom:var(--wp--preset--spacing--40);height:var(--wp--preset--spacing--70)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:spacer {"height":"var:preset|spacing|30","style":{"spacing":{"margin":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}}}} -->
<div style="margin-top:var(--wp--preset--spacing--30);margin-bottom:var(--wp--preset--spacing--30);height:var(--wp--preset--spacing--30)" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->Second, the native block save unexpectedly swaps the height value in style from a CSS custom property value to the block attribute syntax. This results in the blocks containing invalid syntax after saving the post in the native editor.
Example Diff
diff --git a/spacer-blocks.html b/spacer-blocks.html
index 5695c405..6a0b0721 100644
--- a/spacer-blocks.html
+++ b/spacer-blocks.html
@@ -1,11 +1,11 @@
<!-- wp:spacer {"height":"var:preset|spacing|70","style":{"spacing":{"margin":{"top":"var:preset|spacing|40","bottom":"var:preset|spacing|40"}}}} -->
-<div style="margin-top:var(--wp--preset--spacing--40);margin-bottom:var(--wp--preset--spacing--40);height:var(--wp--preset--spacing--70)" aria-hidden="true" class="wp-block-spacer"></div>
+<div style="margin-top:var(--wp--preset--spacing--40);margin-bottom:var(--wp--preset--spacing--40);height:var:preset|spacing|70" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:spacer {"height":"var:preset|spacing|30","style":{"spacing":{"margin":{"top":"var:preset|spacing|30","bottom":"var:preset|spacing|30"}}}} -->
-<div style="margin-top:var(--wp--preset--spacing--30);margin-bottom:var(--wp--preset--spacing--30);height:var(--wp--preset--spacing--30)" aria-hidden="true" class="wp-block-spacer"></div>
+<div style="margin-top:var(--wp--preset--spacing--30);margin-bottom:var(--wp--preset--spacing--30);height:var:preset|spacing|30" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
<!-- wp:spacer {"height":"var:preset|spacing|110"} -->
-<div style="height:var(--wp--preset--spacing--110)" aria-hidden="true" class="wp-block-spacer"></div>
+<div style="height:var:preset|spacing|110" aria-hidden="true" class="wp-block-spacer"></div>
<!-- /wp:spacer -->
\ No newline at end of fileAre you able to replicate these two issues?
… splitting by math symbols, this would break for complex values like nested math expressions. Now it will split the string by every number value with a unit, that way it will match all values and convert them to pixel values, to then trigger the math calculation. It will also take into account math expressions wrapped within CSS expressions and take into account this cases.
|
Flaky tests detected in 391625a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4254997932
|
|
@dcalhoun Thank you for testing and I'm sorry for taking a bit to go back to this PR 😅
Nice catch! You are right, complex values with nested values caused an error within the parser so it would take the I've updated it in 16155cc That change updates the Let me know what you think of that approach, unit tests pass correctly so I don't expect any regressions, I've tested the font size picker and it's working as expected.
Interesting, I've tried to paste the example HTML code, and save the post and the content doesn't change. What are the exact steps to reproduce? Let me know. Thanks again! |
Thanks for requesting the reproduction steps. It made me realize the nuance of my experience after exploring it again. It appears the issue I described — swapping of the
However, I noticed while using this branch to test the web editor the console displays invalid block warnings. The invalid block warnings appear directly related to the original issue I described. I am unsure what the desired outcome in the web editor is — if the warnings are an issue or expected.
|
There was a problem hiding this comment.
The code appears to be function as expected. I wanted to await your response to my previous comment regarding the web editor before providing an approval. Let me know what you think.
| for ( const unit of cssUnitsBits ) { | ||
| // The following regex matches numbers that have a following unit | ||
| // E.g. 5.25rem, 1vw | ||
| const regex = /\d+\.?\d*[a-zA-Z]+|\.\d+[a-zA-Z]+/g; | ||
| cssUnit = cssUnit.replace( regex, ( foundUnit ) => { |
There was a problem hiding this comment.
Let me know what you think of [the updated
evalMathExpression] approach, unit tests pass correctly so I don't expect any regressions, I've tested the font size picker and it's working as expected.
I reviewed 16155cc and the changes make sense to me. 👍🏻
The only thing that gave me pause was the change from for loop to String.prototype.replace, namely because the remainder of the function continues to rely upon a for loop. It isn't a big deal, it just feels a bit odd to mix the functional and imperative coding paradigms in the same function.
WDYT? It is fine with me to keep the code as-is. I merely wanted to seek your thoughts.
There was a problem hiding this comment.
Since it's been a bit since I did this change, I'll review this code and let you know.
There was a problem hiding this comment.
After reviewing the code I brought back the usage of the for loop in 391625a
It also added a fix for the issue you mentioned in this comment. Now we should support nested calc() usages like clamp(2.625rem, calc(2.625rem + ((1vw - 0.48rem) * 8.4135)), 3.25rem) which is the current value for the H2 tag for the Twenty Twenty-Three theme.
|
Semi-related: I noticed that the failed CSS unit parsing occurs for the Twenty Twenty-Three I do not plan to create a new issue to track the font size issue, but let me know if you would prefer me create an issue. |
|
Hey @dcalhoun 👋 Thank you for sharing the steps! The idea of this PR is to support posts created from the Web Editor when they release the functionality to add Spacing presets to the Spacer block. Testing the web editor without their changes will display block warnings or invalid formats because in this branch or in the production version they don't have this new functionality. So for this PR specifically, we should focus on testing if adding the snippets from the PR description works correctly and don't break the editor. So we can ship this and have it for at least two app versions, then the web team can ship their changes on the next WordPress version and the app will be ready for this new format coming from the web. Since we don't support the spacing tool, the mobile editor will continue to create the simple format for the Spacer block and won't break the web editor. I'd say this is an experiment by shipping it before the web editor does to avoid having to work with feature flags like we've done before for the Quote or List blocks. Let me know if you have any doubts or concerns about it, thanks! |
That makes sense. Thank you for expressing that in this way. I also verified that understanding by merging #44002 into this branch and validating the web editor does function as expected without block validation warnings.
You have addressed all my concerns. Thank you! |
…ted calc(). It also brings back the usage of the for loop to keep consistency instead of using replace directly from the string.
geriux
removed
the
Mobile App - Automation
Related PRs:
Closes #44234
What?
This PR adds parsing support for preset values for the
Spacerblock.Why?
The idea is to support parsing/rendering these presets values before the Web editor lands these changes. If the Web editor adds these changes first, the mobile editor would not be able to render the block with these attributes.
Note: This PR does not add the functionality to set a spacing preset value, for now, it will only have the units picker. In the future, we should add this new picker as the Web editor will add it later on.
How?
First, we are passing the spacing values coming from the theme. This will enable getting the spacing values set in the Spacer block.
We are adding a custom
save.native.jsfile, which is the same one that will be added in the web PR. This is needed to be able to read the style properties correctly. Once the web PR lands, we can remove this native implementation of thesave.jsfile.For the Spacer block, we are now checking for available spacing sizes from the theme, which will be used for the
getCustomValueFromPresetutil that will return the correct value if it exists.These will be stored in the
presetValuesobject that is now being passed to theControlscomponent, I decided to pass these values so we avoid having to get them in that component as well.If a Spacer block has a spacing variable that doesn't exist in the current theme, it will use the default pixel value.
This PR also adds some integration tests for cases with a Spacer block with spacing preset values with and without theme data.
Testing Instructions
Case 1 - Adding a Spacer block with existing spacing preset values in the theme
Precondition: Use a theme with spacing values, Masu, Twenty Twenty-Three.
Example Spacer code:
Note: Make sure the spacing value e.g.
--wp--preset--spacing--70exists in your current theme.100Case 2 - Adding a Spacer block with non-existing spacing preset values in the theme
Precondition: Use a theme with spacing values, Masu, Twenty Twenty-Three.
Example Spacer code:
Note: Make sure the spacing value e.g.
--wp--preset--spacing--110does not exist in your current theme.100Testing Instructions for Keyboard
N/A
Screenshots or screencast