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

Fix class serialization of font-size and colors in dynamic blocks that use block supports #35751

Merged
merged 6 commits into from
Oct 20, 2021

Conversation

oandregal
Copy link
Member

@oandregal oandregal commented Oct 19, 2021

Fixes #35708

Converting the slug to a kebab-case format has been problematic in the past. In the client, we use lodash's kebabCase, which has some peculiarities. In the server, we don't have that, so we created gutenberg_experimental_to_kebab_case function to replicate the most common use cases. See #32347 for details.

Apparently, when that was implemented the block supports didn't get updated. Hence, blocks that are rendered in the server may still generate an incorrect class. This PR fixes the font size class generation.

How to test

  • Use the empty theme and register this font size:
{
    "settings": {
        "typography": {
            "fontSizes": [
                {
                    "slug": "h1",
                    "size": "32px",
                    "name": "Used in H1 titles"
                }
            ]
        }
    }
}
  • Add a paragraph, a post title, and a site title in the block editor.
  • Attach the font size to those blocks.

The expected result at this point is that, in the editor, they have a new class called .has-h-1-font-size.

  • Publish the post and verify that, in the front end, they still have that class (previous to this PR it'd be rewritten to .has-h1-font-size, which is incorrect as per lodash's kebabCase function).

Related

Font family preset is being fixed at #31910

@oandregal oandregal self-assigned this Oct 19, 2021
@oandregal oandregal requested a review from ntsekouras October 19, 2021 09:14
@oandregal oandregal added the [Type] Bug An existing feature does not function as intended label Oct 19, 2021
@oandregal oandregal changed the title Fix class serialization in dynamic blocks that use block supports Fix class serialization of font-size in dynamic blocks that use block supports Oct 19, 2021
@oandregal
Copy link
Member Author

Pushed a fix for font family at 21e6ab1

@oandregal oandregal changed the title Fix class serialization of font-size in dynamic blocks that use block supports Fix class serialization of font-size and colors in dynamic blocks that use block supports Oct 19, 2021
@oandregal
Copy link
Member Author

Pushed a fix for colors as well. Also pushed tests for both typography and colors.

@oandregal oandregal force-pushed the fix/font-size-class-in-block-supports branch from 9b67881 to 53b1991 Compare October 19, 2021 10:39
@ockham
Copy link
Contributor

ockham commented Oct 19, 2021

PHP Unit tests are failing 😕

There was 1 failure:

1) WP_Block_Supports_Spacing_Test::test_spacing_gap_block_support_renders_block_inline_style
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div style="--wp--style--block-gap: 3em" class="wp-block-test-block">Test</div>'
+'<div class="wp-block-test-block">Test</div>'

/var/www/html/wp-content/plugins/gutenberg/phpunit/block-supports/spacing-test.php:52

@ockham
Copy link
Contributor

ockham commented Oct 19, 2021

Sorry for the naive question, but how do I register a font size? Copy-paste the snippet to the theme's theme.json? 😅

@ockham
Copy link
Contributor

ockham commented Oct 19, 2021

Sorry for the naive question, but how do I register a font size? Copy-paste the snippet to the theme's theme.json? 😅

It's experimental-theme.json for Emptytheme, but regardless, that didn't work. (Is that expected to work?)

Adding to Gutenberg's own lib/theme.json also didn't work 😕 (I rebuilt and re-started wp-env.)

diff --git a/lib/theme.json b/lib/theme.json
index f32734b6ef..0dd7f4a0e5 100644
--- a/lib/theme.json
+++ b/lib/theme.json
@@ -185,6 +185,11 @@
                        "customTextDecorations": true,
                        "customLetterSpacing": true,
                        "fontSizes": [
+                               {
+                                       "slug": "h1",
+                                       "size": "32px",
+                                       "name": "Used in H1 titles"
+                               },
                                {
                                        "name": "Small",
                                        "slug": "small",

image

@ockham
Copy link
Contributor

ockham commented Oct 19, 2021

Ah-ha, it seems like this is only available in the Site Editor!

image

That doesn't seem to apply at block level though 🤔 Changing it doesn't seem to do much at all, actually.

Oh well 😅 Sorry I cannot be of more help for now, but I guess I need further instructions 😅

@oandregal oandregal force-pushed the fix/font-size-class-in-block-supports branch from 53b1991 to f9c4700 Compare October 20, 2021 09:22
@oandregal
Copy link
Member Author

oandregal commented Oct 20, 2021

It's experimental-theme.json for Emptytheme, but regardless, that didn't work. (Is that expected to work?)

Do you perhaps need to update the theme-experiments repo to the latest? We no longer support experimental-theme.json, only theme.json. The empty theme has been updated a few moons ago. What is probably happening in your setup is that, because the theme has the wrong file, the theme settings are ignored and, instead, the core defaults for font size are shown.

I'm looking now at the failing test.

@oandregal
Copy link
Member Author

Pushed a change to fix the test.

@oandregal oandregal added the [Feature] Block API API that allows to express the block paradigm. label Oct 20, 2021
@ockham
Copy link
Contributor

ockham commented Oct 20, 2021

It's experimental-theme.json for Emptytheme, but regardless, that didn't work. (Is that expected to work?)

Do you perhaps need to update the theme-experiments repo to the latest? We no longer support experimental-theme.json, only theme.json. The empty theme has been updated a few moons ago. What is probably happening in your setup is that, because the theme has the wrong file, the theme settings are ignored and, instead, the core defaults for font size are shown.

Ah, that seems to have done the trick 🤦‍♂️ I thought wp-env would take care of that for me, but turns out I had to manually git checkout trunk && git pull in ~/.wp-env/[sha]/theme-experiments. Thanks André!

image

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

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

Thanks André, this is working great now 👍

Love the unit test coverage. I only had two minor notes -- feel free to address or ignore 😄

@oandregal
Copy link
Member Author

I see there's the React Native e2e test failure. I've seen this one in another of my PRs and the recommendation was to re-run the test until it passed. I want to use parts of this PR (test) on #31910 so I'm going to go ahead and merge this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Bug An existing feature does not function as intended
Projects
None yet
2 participants