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

Different class names for preset slugs #32347

Closed
oandregal opened this issue May 31, 2021 · 14 comments · Fixed by #32352, #32742 or #32766
Closed

Different class names for preset slugs #32347

oandregal opened this issue May 31, 2021 · 14 comments · Fixed by #32352, #32742 or #32766
Assignees
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. 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] Bug An existing feature does not function as intended

Comments

@oandregal
Copy link
Member

oandregal commented May 31, 2021

Themes provide some presets to the editor, such as font sizes and colors, either via the add_theme_support #26940 or the theme.json #31629

The slug of the preset values is used to generate a class name in two different situations:

  1. when in the editor, to generate the class name to be attached to the block markup upon an user action, as in:
<p class="has-slug-preset">Content</p>
  1. to generate the classes to be enqueued as part of the global styles stylesheet:
.has-slug-preset { ... }

What's the issue

When the slug contains a number, the class attached to the block markup and the class generated for the stylesheet are different. See issues reported at #31629 and #26940 There's also issues with slugs that contain camelCased words.

Slug Class generated in the editor Class generated in the front
slug1 .has-slug-1-preset .has-slug1-preset
2slug .has-2-slug-preset .has-2slug-preset
slug3number .has-slug-3-number-preset .has-slug3number-preset
4th .has-4th-preset .has-4th-preset
5sl .has-5-sl-preset .has-5sl-preset
slugNumber .has-slug-number-preset .has-slugnumber-preset

Notice how the server doesn't process the slug but the client splits by number, except if the slug is an English ordinal such as 4th. The client also converts camelCased slugs into kebab-cased slugs, while the server doesn't.

Why this happens

In the client, both for font-sizes and colors we use the lodash's kebabCase function to process the incoming slug such as:

has-${ kebabCase( slug ) }-preset

The lodash's kebabCase function has the particularity that it considers numbers as something to separate as well, unless they're English ordinals.

In the server, we don't process the slug but output it as it is:

'.has-' . $value['slug'] . '-' . $class['class_suffix']

How to fix it

  1. Do not do any processing to the slug in the client, but use the incoming value. Existing classes would still work: the old slug is recognized as an extra class and because the theme should have enqueued it, it'll still work.

  2. Replicate the lodash's kebabCase logic (including English ordinals) in the server.

  3. Use our own regexp / function to do the same in both places, to make sure changes to lodash's kebabCase don't affect us.

@oandregal oandregal added [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. CSS Styling Related to editor and front end styles, CSS-specific issues. Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Type] Bug An existing feature does not function as intended labels May 31, 2021
@oandregal
Copy link
Member Author

oandregal commented May 31, 2021

This seems that has been in Gutenberg for a long time: I hadn't seen reported before, so either theme authors adapted to this quirk or nobody had run into this yet.

I'd think I'd discard 2, as I don't think the lodash's kebab case logic for numbers is something consistent or easy to understand. Also, porting the same behavior to the server may prove problematic should the lodash's behavior change to add new cases.

1 vs 3. We take slugs, which is something people already provide normalized (using - to separate the words). However, it seems that some processing could be useful (make sure the slug is always lowercase, for example). So, I'm inclined to think the expected behavior would be to make sure they're lowercase but do nothing else (no camelCase to kebab-case, etc).

cc @jorgefilipecosta @youknowriad for thoughts.

@oandregal oandregal changed the title Different class names for preset slugs that contain numbers Different class names for preset slugs May 31, 2021
@jorgefilipecosta
Copy link
Member

I think we should just remove has-${ kebabCase( slug ) }-preset, from the client and do any processing on the slug. Slugs were expected to be used as is in the class.

@oandregal
Copy link
Member Author

oandregal commented May 31, 2021

Note that by doing 1 (or 3, for that matter), existing blocks will still work as expected:

  • The existing class will still be attached in the block markup, hence, if the theme enqueues the class (which it has to do, otherwise it wouldn't have been working already)
  • The block in the editor appears as expected. The old class will become part of the block sidebar free-form class input.

Going to implement 1 then. We can refine after if necessary.

@oandregal
Copy link
Member Author

oandregal commented May 31, 2021

I've prepared #32352 to remove the processing from the client.

However, I'm having second thoughts about removing the processing entirely. It feels we should make sure classes don't have whitespace. If the slug contains whitespace there may be other things that break, I haven't checked. Anyway, making sure the classes the system generates don't break because they have whitespaces seems something to care about.

@oandregal
Copy link
Member Author

Reopening as we have to revert the PR that fix it #32742

@ockham
Copy link
Contributor

ockham commented Jun 16, 2021

I think it's fine to continue to use kebabCase in the editor. On the frontend, let's aim to cover the case where a number is followed by a string (slug1 -> slug-1) or vice versa (2slug -> 2-slug), or both; and maybe the camel case conversions.

It's probably okay to ignore the special treatment of ordinals for now -- it's enough of a edge case to tell people "don't use those" maybe 😬

@ockham
Copy link
Contributor

ockham commented Jun 16, 2021

We can approach this by adding unit test cases (both JS and PHP) for the scenarios described in #31629 and #26940.

@justintadlock
Copy link
Contributor

So, has-font-size-2xl is reverting to the broken has-font-size-2-xl on the front end now? I've had to maintain a hacky workaround for my theme users to fix this. I literally just sent out an update since it was fixed in the last Gutenberg release.

If a slug is not actually invalid, it should not be altered.

Can we get a flag or hook for handling this on our own?

@oandregal
Copy link
Member Author

Got an in-progress PR to fix this at #32766

@oandregal
Copy link
Member Author

@justintadlock I hear your frustration and I feel it myself. I wish we didn't have to do this.

To clarify direction: the front and the site editor are going to behave like the post editor has been behaving since the beginning. This is, a slug such as 2xl for font sizes is going to become the class has-2-xl-font-size and a slug such as 2nd is going to become has-2nd-font-size. The exact behaviour can be tested at lodash's kebabCase REPL (see).

@justintadlock
Copy link
Contributor

I'm just asking that we have an easy way for those of who wish to fix this on our end be able to do so. Please just add filter hooks in the appropriate areas as part of the changes or a way to flag a slug with a number in it from being mangled.

@justintadlock
Copy link
Contributor

I'm not really familiar with the JavaScript side of things, but looking over the JS hooks system, something like this could suffice for colors:

// Original
return `has-${ kebabCase( colorSlug ) }-${ colorContextName }`;

// New
return applyFilters( 
	'colorPresetSlug',
	`has-${ kebabCase( colorSlug ) }-${ colorContextName }`,
	colorSlug
);

And, for font sizes:

// Original
return `has-${ kebabCase( fontSizeSlug ) }-font-size`;

// New
return applyFilters( 
	'fontSizePresetSlug', 
	`has-${ kebabCase( fontSizeSlug ) }-font-size`,
	fontSizeSlug
);

The proposed gutenberg_experimental_to_kebab_case() function would also have a similar hook.

@jorgefilipecosta
Copy link
Member

Hi @justintadlock, thank you for suggesting a possible fix using filters.

The problem with a filter-based approach is that given that classes are part of the blocks markup as soon as the plugin that has this filter is installed all the blocks created with the old markup before the filter was there don't match the new markup and would be considered invalid. The same after a plugin is disabled the markup produced for a block without the plugin would be different and blocks created while the plugin was active would become invalid after the plugin is disabled.

@justintadlock
Copy link
Contributor

I was more thinking the filter would come from a theme, but it doesn't really matter.

Are you saying that the block markup would literally be invalid in terms of creating an error? Or, are you saying that simply the markup would be different?

I have seen no block invalidation between the time before the fix was added, the time since the fix went in, and the time of the reversal of the fix. The classes are merely different. Now, I have sites (and some of my theme users have sites) with different classes. And, the more recent "fix" of the whole system looks like it will break my earlier front-end correction of the markup. Of course, I can probably just address this with some extra bloat to my CSS for covering both cases.

It seems to me to be no different than when users switch between one theme and the next, each with their own slugs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CSS Styling Related to editor and front end styles, CSS-specific issues. [Feature] Themes Questions or issues with incorporating or styling blocks in a theme. 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] Bug An existing feature does not function as intended
Projects
None yet
4 participants