fix(Dropdown): fix key handling for options #1639
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1639 +/- ##
==========================================
+ Coverage 99.75% 99.75% +<.01%
==========================================
Files 141 141
Lines 2406 2408 +2
==========================================
+ Hits 2400 2402 +2
Misses 6 6
Continue to review full report at Codecov.
|
dvdzkwsk
force-pushed
the
fix/dropdown-keys
branch
2 times, most recently
from
c4194fa
to
af69e96
Compare
| {_.map(options, option => ( | ||
| <option key={option.value} value={option.value}>{option.text}</option> | ||
| {_.map(options, (option, i) => ( | ||
| <option key={option.key || option.value} value={option.value}>{option.text}</option> |
There was a problem hiding this comment.
To retain parity with the generated DropdownItem this should likely include the -${index} suffix when using the value as the key. I originally included that fix in this change, but then redacted it since I'd prefer we stop using indexes in keys and require users to provide their own when values are non-unique.
There was a problem hiding this comment.
Yep. In fact, this should use a factory which already requires users to use keys when passing objects or elements. We use literal values for the key otherwise.
See factories.js where we should likely add an HTML factory for option:
https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/lib/factories.js#L125
src/modules/Dropdown/Dropdown.js
Outdated
| @@ -1152,7 +1152,7 @@ export default class Dropdown extends Component { | |||
|
|
|||
| return _.map(options, (opt, i) => ( | |||
| <DropdownItem | |||
| key={`${opt.value}-${i}`} | |||
| key={opt.key || `${opt.value}-${i}`} | |||
There was a problem hiding this comment.
Again here, we should add the create method to the DropdownItem. We have a factory creator for this:
DropdownItem.js
Dropdown.create = createShorthandFactory(Dropdown, val => ({ text: val, value: val }))Then:
_.map(options, opt => Dropdown.create(opt)) // PLUS all the other props here...Tests:
DropdownItem-test.js
common.implementsCreateMethod(Dropdown)There was a problem hiding this comment.
Did not know, will take a look. You mean DropdownItem.create though, right?
There was a problem hiding this comment.
Whoops. Yes, sir.DropdownItem.create.
dvdzkwsk
force-pushed
the
fix/dropdown-keys
branch
from
af69e96
to
b2aacc9
Compare
Fixes "key" prop not being respected on options provided to <Dropdown />
dvdzkwsk
force-pushed
the
fix/dropdown-keys
branch
from
998acf7
to
90efae1
Compare
dvdzkwsk
force-pushed
the
fix/dropdown-keys
branch
from
90efae1
to
04ef42b
Compare
|
Released in |
Fixes #1634