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

Addon-controls: Fix duplicate color swatch id's in Color control #14925

Conversation

Tomastomaslol
Copy link
Member

Issue: #14793

What I did

  • Changed key to {preset.value}-${index} for looped Swatches to avoid duplicate keys

How to test

Before change

Kapture.2021-05-14.at.12.09.17.mp4

After change

Kapture.2021-05-14.at.12.12.08.mp4

@nx-cloud
Copy link

nx-cloud bot commented May 14, 2021

Nx Cloud Report

CI ran the following commands for commit c41c801. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

Status Command
#000000 nx run-many --target=prepare --all --parallel --max-parallel=15

Sent with 💌 from NxCloud.

@nx-cloud
Copy link

nx-cloud bot commented May 14, 2021

Nx Cloud Report

We didn't find any information for the current pull request with the commit c41c801.
Please make sure you set the \ NX_BRANCH\ environment variable in your CI pipeline .

Check the Getting started section to configure the app.


Sent with 💌 from NxCloud.

<WithTooltip
key={preset.value}
// eslint-disable-next-line react/no-array-index-key
key={`${preset.value}-${index}`}
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think there is a way to get unique keys without using the index.

type ParsedColor = {
  valid: boolean;
  value: string;
  keyword: string;
  colorSpace: ColorSpace;
  [ColorSpace.RGB]: string;
  [ColorSpace.HSL]: string;
  [ColorSpace.HEX]: string;
};

Is there a better to solve this? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. The "optimized" way would be to only de-dupe when there are actual duplicates, so a,b,c,b might become a,b1,c,b2, but I don't think it's worth the effort

@Tomastomaslol Tomastomaslol changed the title Fix: add index to key when looping colour swatches in Color control Fix: Add index to key when looping colour swatches in Color control May 14, 2021
<WithTooltip
key={preset.value}
// eslint-disable-next-line react/no-array-index-key
key={`${preset.value}-${index}`}
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine. The "optimized" way would be to only de-dupe when there are actual duplicates, so a,b,c,b might become a,b1,c,b2, but I don't think it's worth the effort

@shilman shilman removed the P0 label May 14, 2021
@shilman shilman changed the title Fix: Add index to key when looping colour swatches in Color control Addon-controls: Fix duplicate color swatch id's in Color control May 14, 2021
@shilman shilman merged commit 0e94fa7 into storybookjs:next May 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants