PaletteEdit: Fix order numbers #52212

megane9988 · 2023-07-02T06:33:48Z

What?

For #51230

Why?

copilot:summary

How?

Change Site Language Setting English to 日本語

Go to Style Color setting
Push the Custom（カスタム） Plus icon

You can find Custom Color (the name is 色 1)
Add some custom colors
You can watch Color name (incremental)

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

megane9988 · 2023-07-02T06:34:01Z

It is a draft now.

ramonjd · 2024-03-07T03:34:29Z

Thanks for the PR @megane9988

Could you rebase this PR? It would also help to have a description of how to test.

Thank you!

github-actions · 2024-03-07T03:34:40Z

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: megane9988 <megane9988@git.wordpress.org>
Co-authored-by: ramonjd <ramonopoly@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: mirka <0mirka00@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

packages/components/src/palette-edit/index.tsx

ramonjd · 2024-03-07T07:16:50Z

Thanks for the update to this PR @megane9988 and for the tests 🙇🏻

So to summarize: Gutenberg was trying to create slugs for colors using translated strings, e.g., 色 6 but that didn't pass the regex new RegExp( ^${ slugPrefix }color-([\d]+)$ )

This is working pretty well for me.

t-hamano

Thanks for the PR!

This PR is working as expected and will also resolve two issues, #55185 and #51230.

7c6c80130e8a3a5ecbc25d0896d0aee6.mp4

As a follow-up, we should apply the same logic not only when a new color is "added" but also when a color label is "updated". As the screencast below shows, after an item with the correct slug is added, reapplying the same label will result in the incorrect slug being applied.

c408a0d4cd2bf3cbffb42f07b3925409.mp4

This means that we apply the getNameAndSlugForPosition() function here as well and completely stop the slug generation logic from relying on the color label:

gutenberg/packages/components/src/palette-edit/index.tsx

Lines 226 to 232 in 33056a4

    
           onChange( { 
        
           	...element, 
        
           	name: nextName, 
        
           	slug: 
        
           		slugPrefix + 
        
           		kebabCase( nextName ?? '' ), 
        
           } )

t-hamano · 2024-03-11T08:13:52Z

packages/components/CHANGELOG.md

+### Bug Fix
+-   `PaletteEdit`: Fix order numbers for color palette ([#52212](https://github.com/WordPress/gutenberg/pull/52212)).


This part is causing a conflict, so could you rebase it again?

I think something like "PaletteEdit: Fix number incrementing of default names for new colors added in non-en-US locales" would be more helpful here, given the specific scope of the fix.

packages/components/src/palette-edit/index.tsx

packages/components/src/palette-edit/test/index.tsx

ramonjd · 2024-03-13T00:11:29Z

As a follow-up, we should apply the same logic not only when a new color is "added" but also when a color label is "updated".

Is it much effort to include it in this PR? It would be good to fix the flow in one go. But also happy for it to be a quick follow up

t-hamano · 2024-03-13T11:17:19Z

Is it much effort to include it in this PR? It would be good to fix the flow in one go. But also happy for it to be a quick follow up

Regarding that issue, I think it's okay to address it in a follow-up since we probably need to add more tests. I plan to address it as part of a tracking issue, #57309.

mirka

Looks ready to merge as an iterative improvement, once @t-hamano's feedback is addressed 👍

The renaming case may be a bit more complicated than the new color case, so I understand why we'd want to do that in a follow up. We also need to figure out which part of the chain is responsible for the name/slug validation (deduplication) logic — I kind of think it has to be provided by the consumer rather than the PaletteEdit component itself, but that's a separate discussion. Maybe it will turn out that the consumer needs to be responsible for the entire name/slug generation logic as well.

WordPress#52212 (comment)

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

ramonjd · 2024-03-18T21:26:41Z

Thanks for the teamwork here, folks.

@megane9988 are you okay to look at a follow up? Otherwise I can add it to my TODO list for later.

Thank you! 🍺

Using FSE to change styles on a page template. When using the English language in site and add new colors to Styles, the default name appear with numbers in order. However, when switching to Japanese language, the sequential order numbers did not work anymore in the default name. Gutenberg was trying to create slugs for colors using translated strings, e.g., 色 6 but that didn't pass the regex new RegExp( ^${ slugPrefix }color-([\d]+)$ ) This commit hardcodes the English `color-` slug fragment to avoid this. Updates unit tests. --------- Co-authored-by: megane9988 <megane9988@git.wordpress.org> Co-authored-by: ramonjd <ramonopoly@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: mirka <0mirka00@git.wordpress.org>

megane9988 requested a review from ajitbohra as a code owner July 2, 2023 06:33

megane9988 marked this pull request as draft July 2, 2023 07:02

megane9988 marked this pull request as ready for review October 20, 2023 05:24

miminari added Global Styles Anything related to the broader Global Styles efforts, including Styles Engine and theme.json [Package] Components /packages/components labels Oct 20, 2023

ramonjd added the [Type] Bug An existing feature does not function as intended label Mar 7, 2024

ramonjd assigned megane9988 Mar 7, 2024

megane9988 closed this Mar 7, 2024

megane9988 force-pushed the feature/fix-sequential-order-numbers-for-color-palette branch from 47aac7f to 9888332 Compare March 7, 2024 05:49

megane9988 reopened this Mar 7, 2024

ramonjd requested a review from t-hamano March 7, 2024 06:39

ramonjd reviewed Mar 7, 2024

View reviewed changes

packages/components/src/palette-edit/index.tsx Outdated Show resolved Hide resolved

t-hamano mentioned this pull request Mar 11, 2024

PaletteEdit, ColorPalette: Fixes and improvements #57309

Open

9 tasks

t-hamano reviewed Mar 11, 2024

View reviewed changes

t-hamano requested a review from a team March 11, 2024 08:25

mirka approved these changes Mar 18, 2024

View reviewed changes

mirka changed the title ~~fix order numbers for color palette~~ PaletteEdit: Fix order numbers Mar 18, 2024

mirka added the Internationalization (i18n) Issues or PRs related to internationalization efforts label Mar 18, 2024

megane9988 added 3 commits March 19, 2024 00:03

rebase from trunk

5c7b6b1

fix test

fcc6d9d

Response to reviews

6635428

WordPress#52212 (comment)

megane9988 force-pushed the feature/fix-sequential-order-numbers-for-color-palette branch from 33fe907 to 6635428 Compare March 18, 2024 15:18

Apply suggestions from code review

ad75176

Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com>

megane9988 added 2 commits March 19, 2024 00:20

Update CHANGELOG.md

0a8653f

fix doc format

99903e0

ramonjd merged commit b81d990 into WordPress:trunk Mar 18, 2024
55 checks passed

github-actions bot added this to the Gutenberg 18.0 milestone Mar 18, 2024

This was referenced Mar 19, 2024

Custom colors in color palette don't increment on non-English sites #55185

Closed

Default name when adding new colors in styles does not appear in sequential order numbers #51230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PaletteEdit: Fix order numbers #52212

PaletteEdit: Fix order numbers #52212

megane9988 commented Jul 2, 2023 •

edited

Loading

megane9988 commented Jul 2, 2023 •

edited

Loading

ramonjd commented Mar 7, 2024

github-actions bot commented Mar 7, 2024 •

edited

Loading

ramonjd commented Mar 7, 2024

t-hamano left a comment

t-hamano Mar 11, 2024

mirka Mar 18, 2024

ramonjd commented Mar 13, 2024

t-hamano commented Mar 13, 2024

mirka left a comment

ramonjd commented Mar 18, 2024

	onChange( {
	...element,
	name: nextName,
	slug:
	slugPrefix +
	kebabCase( nextName ?? '' ),
	} )

		### Bug Fix
		- `PaletteEdit`: Fix order numbers for color palette ([#52212](https://github.com/WordPress/gutenberg/pull/52212)).

PaletteEdit: Fix order numbers #52212

PaletteEdit: Fix order numbers #52212

Conversation

megane9988 commented Jul 2, 2023 • edited Loading

What?

Why?

How?

Testing Instructions

Testing Instructions for Keyboard

Screenshots or screencast

megane9988 commented Jul 2, 2023 • edited Loading

ramonjd commented Mar 7, 2024

github-actions bot commented Mar 7, 2024 • edited Loading

ramonjd commented Mar 7, 2024

t-hamano left a comment

Choose a reason for hiding this comment

t-hamano Mar 11, 2024

Choose a reason for hiding this comment

mirka Mar 18, 2024

Choose a reason for hiding this comment

ramonjd commented Mar 13, 2024

t-hamano commented Mar 13, 2024

mirka left a comment

Choose a reason for hiding this comment

ramonjd commented Mar 18, 2024

megane9988 commented Jul 2, 2023 •

edited

Loading

megane9988 commented Jul 2, 2023 •

edited

Loading

github-actions bot commented Mar 7, 2024 •

edited

Loading