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

#3185 fix - Duplicate react component key props #3186

Merged
merged 8 commits into from
Jul 28, 2021

Conversation

jh0l
Copy link
Contributor

@jh0l jh0l commented May 23, 2021

Fix for #3185

Description

Steps to reproduce the behavior:

1. In a venia store with venia sample data, go to any product with colour variants

2. Click on the default colour variant (matches the colour of the selected product's default carousel thumbnails).

3. Click on an alternative colour, then click on the default colour again, repeat this

4. Duplicate thumbnails for the default colour variant will be produced

In the console react will warn that it has detected duplicate key props in the thumbnail list

Expected behavior

Should be no duplicate keys in the carousel's thumbnail list.

  • carousel thumbnail keys are not currently unique based on item file + label

  • item IDs are unique (within a products variants) so should be a good fix

Changes

  • Change ProductImageCarousel Thumbnail key to unique id

Related Issue

Closes #3185

Verification Steps

  1. Go to any product page that has colour swatch variants.
  2. Verify that selecting between the default variant and alternative variants does not produce duplicate react component keys.

Screenshots / Screen Captures

here's a recording of this behaviour

Refer to magento#3185 for reproducing the bug

* Change ProductImageCarousel Thumbnail key to unique id

* carousel thumbnail keys are not currently unique based on item file + label

* item IDs are unique (within a products variants) so should be a good fix
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented May 23, 2021

Messages
📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

Generated by 🚫 dangerJS against 9809c94

tjwiebell
tjwiebell previously approved these changes May 27, 2021
Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

Simple fix, nice work 👍

@tjwiebell tjwiebell added the version: Patch This changeset includes backwards compatible bug fixes. label May 27, 2021
@sirugh
Copy link
Contributor

sirugh commented May 28, 2021

@jh0l thanks for the contribution - would you please sign the Adobe CLA? Without that we cannot use/merge this PR :(

@jh0l
Copy link
Contributor Author

jh0l commented May 29, 2021

Okay I've signed it,

@sirugh
Copy link
Contributor

sirugh commented Jun 1, 2021

Ok thanks for that - I also was curious how to repro this issue - I'm not able to on https://develop.pwa-venia.com/default/athena-tank-dress.html Do I need to change something in the backend?

Signed-off-by: sirugh <rugh@adobe.com>
@sirugh
Copy link
Contributor

sirugh commented Jun 4, 2021

Though I wasn't able to repro the reported bug, I do believe this solution is better than what we had previously. uid is exactly the field we should be using for key, as it is meant to be unique per item within a list. I don't mind approving this and moving it forward.

sirugh
sirugh previously approved these changes Jun 4, 2021
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Approving though I did make some changes (id -> uid).

Signed-off-by: sirugh <rugh@adobe.com>
@jh0l
Copy link
Contributor Author

jh0l commented Jun 5, 2021

Ok thanks for that - I also was curious how to repro this issue - I'm not able to on https://develop.pwa-venia.com/default/athena-tank-dress.html Do I need to change something in the backend?

Yeah there must be some difference in the way the pwa-studio tutorial sets up venia versus how your CI sets up venia. I'm not sure how to see that though.

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Approving this although I technically made a few changes (id -> uid).

@tjwiebell tjwiebell merged commit 04fdcbd into magento:develop Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui Progress: done version: Patch This changeset includes backwards compatible bug fixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug]: Duplicate react component key props produced when selecting variants in product detail carousel
6 participants