PaletteEdit: Consider digits when generating kebab-cased slug #56713
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
t-hamano
added
[Type] Bug
|
Size Change: +134 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
t-hamano
commented
Dec 1, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -6,6 +6,7 @@ | |||
|
|
|||
| - `FormToggle`: fix sass deprecation warning ([#56672](https://github.com/WordPress/gutenberg/pull/56672)). | |||
| - `QueryControls`: Add opt-in prop for 40px default size ([#56576](https://github.com/WordPress/gutenberg/pull/56576)). | |||
| - `PaletteEdit`: Consider digits when generating kebab-cased slug ([#56713](https://github.com/WordPress/gutenberg/pull/56713)). | |||
There was a problem hiding this comment.
Since the problem shouldn't be an issue with the component itself, I added the changelog to the Enhancements section.
t-hamano
commented
Dec 1, 2023
| * | ||
| * @return {string} Kebab-cased string | ||
| */ | ||
| export function kebabCase( str: any ) { |
There was a problem hiding this comment.
I created a new function to take the numbers into account and kebab case them. This is exactly the same function found in the block editor package.
There was a problem hiding this comment.
We should not be duplicating code like that throughout the codebase.
One way we could approach it is by moving some code around. @wordpress/components is a dependency of @wordpress/block-editor, so perhaps we can move it to @wordpress/components, maybe as part of the privately exposed APIs? Then the block editor package can re-export it, or we can change all exports to consume directly from the @wordpress/components package. WDYT?
There was a problem hiding this comment.
so perhaps we can move it to
@wordpress/components, maybe as part of the privately exposed APIs?
Indee, in that case there is a lot of code to change, so it might be better to address it as a separate PR and then come back to this PR.
Moving this code means that the kebabKase() function will disappear from the @wordpress/block-editor package, but since it is a private API, does that mean we don't have to think about backward compatibility?
There was a problem hiding this comment.
A separate PR sounds good. I'm happy to help with reviewing it FWIW 👍
It doesn't have to disappear from the @wordpress/block-editor package FWIW - it can re-export it if you don't want to change any existing dependencies while keeping full backward compatibility. Alternatively, we could just move it and update all imports, but it will likely require some dependency juggling.
There was a problem hiding this comment.
Got it, does this mean that if we take the following approach in the @wordpress/block-editor package, we can reduce the amount of code changes and keep backward compatibility?
import { privateApis as componentsPrivateApis } from '@wordpress/components';
import { unlock } from '../lock-unlock';
export const { kebabCase } = unlock( componentsPrivateApis );There was a problem hiding this comment.
Yes 👍 it is still a compromise, but I'm happy with it since it operates solely with private APIs.
Thanks for considering it!
t-hamano
commented
Dec 1, 2023
| import { normalizeTextString } from '../strings'; | ||
| import { kebabCase, normalizeTextString } from '../strings'; | ||
|
|
||
| describe( 'kebabCase', () => { |
There was a problem hiding this comment.
This test is exactly the same as this one, so I'm wondering if I should add it.
tyxla
previously requested changes
Dec 1, 2023
There was a problem hiding this comment.
I'm strongly against duplicating code like that throughout the codebase.
We already created some duplication as part of the Lodash migration that I'd like to follow up with, and ideally, we should avoid creating more overhead.
I'd suggest that we consider all available alternatives instead.
| * | ||
| * @return {string} Kebab-cased string | ||
| */ | ||
| export function kebabCase( str: any ) { |
There was a problem hiding this comment.
We should not be duplicating code like that throughout the codebase.
One way we could approach it is by moving some code around. @wordpress/components is a dependency of @wordpress/block-editor, so perhaps we can move it to @wordpress/components, maybe as part of the privately exposed APIs? Then the block editor package can re-export it, or we can change all exports to consume directly from the @wordpress/components package. WDYT?
|
Update: #56758 has moved the ecc3b8f45c279fb21c0f482d993b2c6b.mp4 |
|
Flaky tests detected in c296cfa. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7208870425
|
ciampo
approved these changes
Dec 14, 2023
packages/components/CHANGELOG.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| ## Unreleased | |||
|
|
|||
| ### Enhancements | |||
There was a problem hiding this comment.
Should this go under "Bug fix" instead?
There was a problem hiding this comment.
Ok, I'll update before merging 👍
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
* PaletteEdit: Consider digits when generating kebab-cased slug * Update changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #55587
More context here: #55587 (comment)
What?
This PR makes the
PaletteEditcomponent take numbers into account when automatically generating theslugconverted to kebab cases based on the palette name.As a result, for example, when you enter the palette name
MyColor1, the generated slug will change frommy-color1tomy-color-1.Why?
If you create a custom color, a CSS variable will be generated based on that slug. When you apply that custom color to the site, that CSS variable is referenced as the value.
Notice that the CSS variables are different.
The CSS variable name is
my-color-1because the _wp_to_kebab_case() function is used when the CSS variable is generated on the server side, and numbers are also taken into account when converting to kebab case.But the CSS variable applied as value is
color1. When generating a slug from a name in the Global Style custom color palette, we use theparamCase()( askebabCase()) function from thechange-casepackage, but this function does not take numbers into account.How?
We need to unify the kebab-cased slug, with numbers taken into account.
I think this process needs to be done as early as possible. Before the global style entity is saved, i.e. when generating the slug in the palette. This is because custom colors are stored in the database as values like the following, so it is difficult to extract which string is the real slug.
Testing Instructions
MyColor1.Screenshots or screencast
0dbea657002b7f50bbe70b201c47a087.mp4