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

Dropdown: items missing "key" prop, React warning displayed #1710

Closed
mcrawshaw opened this issue May 28, 2017 · 19 comments
Closed

Dropdown: items missing "key" prop, React warning displayed #1710

mcrawshaw opened this issue May 28, 2017 · 19 comments
Labels

Comments

@mcrawshaw
Copy link
Contributor

Steps

  • Update from version 0.68.3 to 0.68.4.
  • Introduce a Dropdown element with more than one option specified. selection needs to be specified also.
  • Run.

Expected Result

No warning from React.

Actual Result

React error:
Warning: Each child in an array or iterator should have a unique "key" props. Check the render method of 'Dropdown'.

Version

0.68.4

@mcrawshaw mcrawshaw changed the title V0.68.4 Dropdown items not including key, react warning displayed V0.68.4 Dropdown items missing "key" prop, React warning displayed May 28, 2017
@mcrawshaw
Copy link
Contributor Author

mcrawshaw commented May 28, 2017

Note, adding a key to each option fixes the issue. For example:

<Dropdown selection options={[key: "1", value: "1", name: "Option 1"]} />

This wasn't the case for 0.68.3. I see a reference to key in the docs, but it is not consistently specified in the examples.

@mcrawshaw
Copy link
Contributor Author

mcrawshaw commented May 28, 2017

Looks to be an intended change. This issue is mentioned in the latest release documentation:
#1634

So does this mean we need key, value, and text now?

@karlludwigweise @davezuko

@dvdzkwsk
Copy link
Member

dvdzkwsk commented May 28, 2017

From my understanding, key is definitely the preferred way, but it should have been nearly backwards compatible due to createShorthandFactory using value as the default if no key exists.

@levithomason I am, however, curious about this line:

} else if (valIsString || valIsNumber) {
. This looks to be checking the wrong value, at least from my initial read through of the code. Are we sure that value is being used correctly as the default? Maybe I missed something in the DropdownItem change?

@jhartma
Copy link

jhartma commented May 29, 2017

I have a small problem, though: react throws the unique key prop error when allowAdditions is defined and I am not sure how to add a key prop there. Seems like the Add entry has no key?

@karlludwigweise
Copy link

This is the reason I did not want to make the change myself. The Dropdown is too big of a component to oversee for a first time change; for me at least. The component should ideally generate its own keys and use those.

@mcrawshaw
Copy link
Contributor Author

@karlludwigweise, no stress, we have a work around.

Let's see what @levithomason says about the factory before we continue.

@layershifter layershifter changed the title V0.68.4 Dropdown items missing "key" prop, React warning displayed Dropdown: items missing "key" prop, React warning displayed May 31, 2017
@layershifter
Copy link
Member

Here is problem with a factory.

It will generate key only when element passed to it as number or string. When element is passed as object, factory expects that user will define key himself.

I will make PR that fixes this.

@levithomason
Copy link
Member

levithomason commented Jun 1, 2017

This is actually not a bug, if you pass a string/number primitive then we'll assume that as they key. Otherwise, if you pass an options object or an element then you need to supply your own key just as is required by React itself.

@mcrawshaw

So does this mean we need key, value, and text now?

Yep. Anytime you use an object or an element in an array you'll need to pass a key.

If you use primitive shorthand (i.e. a string or number) then the shorthand factory will assume that value as the key.

@davezuko
Factories do not use the value prop for keys, that is specific only to the hidden select in the Dropdown. Factories will only use key/childKey to generate keys, no more magic key generation business as we used to have.

Tangent: the hidden select arguably should go away entirely, it is an artifact from the old days.

@jhartma
Thank you, this was an oversight on our part. The addItem object itself does not have a key. I've opened #1727 to resolve this issue.

@levithomason
Copy link
Member

@jhartma fixed in #1727, it will go out on the next release.

@jhartma
Copy link

jhartma commented Jun 1, 2017

@levithomason nice, thanks a lot for the quick fix

@layershifter
Copy link
Member

layershifter commented Jun 1, 2017

@levithomason it seems that you don't understand the problem described there. In #1639 we broken automatic key generation for Dropdown.Item. After this change we always pass an object to the factory, as I said, I proposed there, if you check it's code you will better understand problem.

@levithomason
Copy link
Member

levithomason commented Jun 1, 2017

@layershifter thanks for pointing out my oversight. There are two points here:

  1. Users' are now required to pass key in their options. This is the intended usage and not a bug. It should have been shipped as a breaking change but was not.

  2. Rather than update the single case in the Dropdown to handle valid falsy key values, we should add the isNil check to the factory here. We currently check if (!props.key) ...., this treats falsy shorthand keys as nonexistent.

I'll update the PR you submitted to fix number 2 above. This way, all keys in all components are fixed.

@levithomason
Copy link
Member

Released in semantic-ui-react@0.68.5.

@SidecarMaster
Copy link

The problem still exists. This is the array I passed in as options:
[{key: "react", value: "react", text: "react"}, {key: "redux", value: "redux", text: "redux"}, {key: "udacity", value: "udacity", text: "udacity"}

This is my Dropdown code:

        <Dropdown
          selection
          label={field.label}
          options={options}
          placeholder='Select Category'
          error={errorAttr}
          >
        </Dropdown>

And this is what is generated:
<div label="Select Category " role="listbox" aria-expanded="false" class="ui selection dropdown" tabindex="0"><div class="text" role="alert" aria-live="polite">react</div>...</div>

As you can see in <div class="text" role="alert" aria-live="polite">react</div> It still doesn't have a key even if the object I passed in has a key property.

@levithomason
Copy link
Member

Note, React doesn't render keys to the DOM. They are used internally only. This example works in docs and tests. There must be something in the project using SUIR causing this.

If you create a minimal repo showing the issue, we can investigate further.

@rawberg
Copy link

rawberg commented Jan 12, 2018

Is there a way to specify a different object property as the key, such as the unique id that might be returned from the server? something like key={id} or key={item.id}

@levithomason
Copy link
Member

@rawberg both key usages you've given are valid there. Not sure of your implementation. You might have better luck on Gitter or StackOverflow as this is a usage question.

@rawberg
Copy link

rawberg commented Jan 13, 2018

reading through the source and #1720 it looks specifying a key no longer works.

@levithomason
Copy link
Member

Keys in Dropdown options are still required and still work. If you do not pass a key we will use the value as a fallback.

@Semantic-Org Semantic-Org locked as resolved and limited conversation to collaborators Feb 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

8 participants