Directory

Root Padding CSS Properties should follow the established pattern · Issue #49801 · WordPress/gutenberg · GitHub
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

Root Padding CSS Properties should follow the established pattern #49801

Open
aurooba opened this issue Apr 13, 2023 · 7 comments · May be fixed by #49809
Open

Root Padding CSS Properties should follow the established pattern #49801

aurooba opened this issue Apr 13, 2023 · 7 comments · May be fixed by #49809
Assignees
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.

Comments

@aurooba
Copy link
Member

aurooba commented Apr 13, 2023

What problem does this address?

The process to set padding values in the styles.spacing section of theme.json is as follows:

  "styles": {
    "spacing": {
      "padding": {
        "top": "1rem",
        "right": "1rem",
        "bottom": "1rem",
        "left": "1rem"
      }
    }
  },

This outputs the following CSS Custom Properties:

    --wp--style--root--padding-top: 1rem;
    --wp--style--root--padding-right: 1rem;
    --wp--style--root--padding-bottom: 1rem;
    --wp--style--root--padding-left: 1rem;

However, according to the established pattern which is also documented in the Block Editor Handbook, what should actually have been generated is:

    --wp--style--root--padding--top: 1rem;
    --wp--style--root--padding--right: 1rem;
    --wp--style--root--padding--bottom: 1rem;
    --wp--style--root--padding--left: 1rem;

Because top, right, bottom, and left are keys inside the padding object. Padding effectively is the category here (to use the language of the Block Editor Handbook), and while in CSS, we are used to padding-right, padding-left etc, theme.json should adhere to its own conventions first for consistency, predictability, and a better developer experience.

What is your proposed solution?

Adjust the parser so that the padding CSS Custom Properties (and any others that break established convention like this) are output correctly, with categories and their children being separated by --.

@Mamaduka Mamaduka added the Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json label Apr 13, 2023
@Mamaduka
Copy link
Member

@oandregal, can you share more context behind this decision?

@kathrynwp kathrynwp added the [Type] Enhancement A suggestion for improvement. label Apr 13, 2023
@oandregal
Copy link
Member

@oandregal, can you share more context behind this decision?

Unfortunately, I don't have any.

However, by looking at the code changes, it sounds like this PR is the first that introduced those variables. I'm tentatively pinging the people involved there in case they have any context @tellthemachines @ramonjd @andrewserong Also they are the spacing/padding/layout masterminds, so, hopefully, they'll know their way around.

@aurooba
Copy link
Member Author

aurooba commented Apr 13, 2023

Once we hear back from folks, and if we decide to update this as proposed – if the original code owners don't have the time to address it, I'd be happy to. I come up against this a lot when I'm in the flow of writing my styles and want to reference the generated CSS Custom Properties, and would want to participate in expediting its fix. :)

Edit: I connected a tentative PR, anyway. Just in case.

@andrewserong
Copy link
Contributor

Thanks for writing up this issue, it's a good discussion to have!

From a quick skim here are some other CSS variables that our output or used in Gutenberg and theme.json:

--wp--preset--color--vivid-red
--wp--preset--color--light-green-cyan
--wp--preset--font-size--small
--wp--preset--font-size--x-large
--wp--preset--font-family--cambria-georgia
--wp--style--global--content-size
--wp--style--global--wide-size

I don't feel too strongly about it, but from a quick glance, it seems that the use of the CSS property name in the existing variables is consistent with the above, where padding-left is the CSS property to be output, rather than being used to denote the structure within theme.json? Curious to hear what other folks think, too.

One potential issue with exploring renaming the variables is if there are existing themes already referencing the current property names.

@tellthemachines
Copy link
Contributor

The other thing to consider is that it is possible to set single-value padding as a shorthand in theme.json, like so:

"styles": {
		"spacing": {
                        "padding": "32px"
                }
	}

Although the implementation is inconsistent due to not allowing multi-value shorthand (see #40132) this is another reason for the current format.

Regarding back compat, a quick check of WP directory shows some themes are already using these properties, so we'd have to continue supporting the old format even if we changed it.

@aurooba
Copy link
Member Author

aurooba commented Apr 14, 2023

I don't feel too strongly about it, but from a quick glance, it seems that the use of the CSS property name in the existing variables is consistent with the above, where padding-left is the CSS property to be output, rather than being used to denote the structure within theme.json? Curious to hear what other folks think, too.

@andrewserong, I see what you mean there. Another interpretation: let's take --wp--preset--font-size--small, in theme.json for example, there is a key called fontSizes which is extrapolated and turns into font-size for each size in the array, which still follows the idea of camelCase single key in theme.json turning into kebab-case for the CSS Custom Property, and having a -- between two keys.

The other variables still follow the established pattern of a -- between two keys, making any kind of programmatic extrapolation or just plain old remembering how to write something in your CSS much easier because it's the same pattern.

Although the implementation is inconsistent due to not allowing multi-value shorthand (see #40132) this is another reason for the current format.

I'm not sure why being able to set a single value would prohibit us from following the established patterns for the variables. Maybe I'm not understanding the point, could you elaborate more, @tellthemachines?

Regarding back compat, a quick check of WP directory shows some themes are already using these properties, so we'd have to continue supporting the old format even if we changed it.

This is true, the same way the block-gap CSS Custom Property is still supported. I think this is enough of a tripping hazard (or paper cut) that I would personally vote to do it anyway and maintain back compat. But if that's not really something a lot of people feel strongly about, that's okay, too. :)

@tellthemachines
Copy link
Contributor

I'm not sure why being able to set a single value would prohibit us from following the established patterns for the variables. Maybe I'm not understanding the point, could you elaborate more, @tellthemachines?

You mention above that "top, right, bottom, and left are keys inside the padding object" as a reason for the change. I'm pointing out that it's not always the case: padding can also be defined as a string.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
6 participants