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

GradientPicker: use index while iterating over gradient entries to avoid React "duplicated key" warning #57361

Merged
merged 4 commits into from
Dec 26, 2023

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Dec 24, 2023

Part of #57309
Similar to #43096

What?

This PR avoids the React Warning error that occurs when the GradientPicker component renders entries with duplicate styles.

b6ca74abb97903934b2750c8cc1c1e81.mp4

Why?

Now the actual gradient style is used as the key. However, this style is user-definable via theme.json or a custom palette, so duplication can occur.

How?

Use index for the key prop. I've heard that using index as a key is not recommended, but I think we have to use it in this case.

The reason this error does not occur with solid color is that index is used for the key.

Testing Instructions

Please update Emptytheme's theme.json as follows in advance. The first and second in the gradient palette have the same style.

test/emptytheme/theme.json
{
	"$schema": "https://schemas.wp.org/trunk/theme.json",
	"version": 2,
	"settings": {
		"appearanceTools": true,
		"layout": {
			"contentSize": "840px",
			"wideSize": "1100px"
		},
		"color": {
			"gradients": [
				{
					"name": "Gradient 1",
					"gradient": "linear-gradient(135deg,rgba(6,147,227,1) 0%,rgb(155,81,224) 100%)",
					"slug": "gradient-1"
				},
				{
					"name": "Gradient 2",
					"gradient": "linear-gradient(135deg,rgba(6,147,227,1) 0%,rgb(155,81,224) 100%)",
					"slug": "gradient-2"
				},
				{
					"name": "Gradient 3",
					"gradient": "linear-gradient(135deg,rgba(252,185,0,1) 0%,rgba(255,105,0,1) 100%)",
					"slug": "gradient-3"
				}
			]
		}
	}
}
  • Activate Emptytheme.
  • Go to Apperance > Editor > Global Styles > Colors > Palette.
  • Click the "Gradient" tab.
    • trunk: React warning error occurs.
    • This PR: React warning error does not occur.
  • Adds a continuous gradient in a custom palette.
    Note: The first color is transparent, but this is an issue that needs to be fixed separately as reported in this comment.
  • Click "Done" button.
    • trunk: React warning error occurs.
    • This PR: React warning error does not occur.

@t-hamano t-hamano added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Dec 24, 2023
@t-hamano t-hamano self-assigned this Dec 24, 2023
@t-hamano t-hamano requested a review from a team December 24, 2023 04:24
@t-hamano t-hamano marked this pull request as ready for review December 24, 2023 04:28
@@ -46,7 +46,7 @@ function SingleOrigin( {
const gradientOptions = useMemo( () => {
return gradients.map( ( { gradient, name }, index ) => (
<CircularOptionPicker.Option
key={ gradient }
key={ index }
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we can't use the slug? It's marked as required in TS and the JSON schema says it should be a unique identifier.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the custom color palette (PaletteEdit component), this slug is automatically generated from the name entered by the user. Therefore, if a user defines the same color name across multiple palettes, duplicate slugs will occur.

image

Also, as reported in #55185, the numbers do not increase in non-English languages, so slug duplication will occur.

However, this is unexpected behavior and I think it should be improved in the future so that duplication of slugs does not occur in any scenario, so I updated this PR to use the slug as a key as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, thanks for the explanation. I agree the slug collision problem should be handled before the data hits the GradientPicker component 👍

Copy link

Flaky tests detected in 2eff803.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7329196636
📝 Reported issues:

@t-hamano t-hamano merged commit db5bbf2 into trunk Dec 26, 2023
56 checks passed
@t-hamano t-hamano deleted the gradient-picker/fix-duplicate-key branch December 26, 2023 12:12
@github-actions github-actions bot added this to the Gutenberg 17.4 milestone Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants