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

help ensure key uniqueness for token field items #7293

Merged
merged 2 commits into from Apr 10, 2018
Merged

help ensure key uniqueness for token field items #7293

merged 2 commits into from Apr 10, 2018

Conversation

kdoh
Copy link
Member

@kdoh kdoh commented Apr 6, 2018

A stab at resolving #7043.

TokenField was using the value as a key for the list items in the token field list, which in the case of the pulse recipient list was potentially an entire user object. I think that was causing React to swallow the new items during the DOM update given it was logging this message.

Encountered two children with the same key, [object Object]. Child keys must be unique; when two children share a key, only the first child will be used. - Pretty much sums up the behavior we saw.

This is a dumb fix to confirm what needs to be changed as index is somewhat frowned upon as a key (which is why I'm guessing this happened in the first place). There are a couple options as I see it:

  1. Use the v + index solution I did here (probably a bad idea, but hey it works)
  2. Allow the user of the <TokenField /> component to provide a custom key prop and if it's not specified fall back to index.
  3. Something smarter @tlrobinson or someone else comes up with.

@kdoh kdoh requested a review from tlrobinson April 6, 2018 21:04
Copy link
Contributor

@tlrobinson tlrobinson left a comment

Choose a reason for hiding this comment

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

Using the index should be ok for this purpose.

@kdoh kdoh merged commit 0f0382f into master Apr 10, 2018
@kdoh kdoh deleted the issue-7043 branch April 10, 2018 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants