fix(factories): handle falsy `key` values #1729

layershifter · 2017-06-01T08:26:41Z

Fixes #1710.
Fixes #1720.

layershifter · 2017-06-01T08:27:39Z

src/modules/Dropdown/Dropdown.js

@@ -1071,7 +1073,7 @@ export default class Dropdown extends Component {
      <select type='hidden' aria-hidden='true' name={name} value={value} multiple={multiple}>
        <option value='' />
        {_.map(options, (option, i) => (
-          <option key={option.key || option.value} value={option.value}>{option.text}</option>
+          <option key={getKeyOrValue(option.key, option.value)} value={option.value}>{option.text}</option>


As described in #1720, 0 will break the previous condition while it's a valid key.

layershifter · 2017-06-01T08:27:45Z

src/modules/Dropdown/Dropdown.js

@@ -1136,7 +1138,7 @@ export default class Dropdown extends Component {
      const defaultProps = {
        active: item.value === selectedLabel,
        as: 'a',
-        key: item.key || item.value,
+        key: getKeyOrValue(item.key, item.value),


layershifter · 2017-06-01T08:35:09Z

src/modules/Dropdown/Dropdown.js

@@ -1167,6 +1169,7 @@ export default class Dropdown extends Component {
      onClick: this.handleItemClick,
      selected: selectedIndex === i,
      ...opt,
+      key: getKeyOrValue(opt.key, opt.value),


Problem

Before #1639, key was captured from value. In #1639 we refactored this to factory usage, but as we know factory will not create key automatically when it recieves an object and it's expected behaviour.

But, it's breaking change that I think should be reverted, because our users rely on that key can be automatically generated from a value prop (#1710).

You are correct, however, this is a semver fail on my part. I should have incremented the minor (since we're pre 1.0).

I do not think we should ship this feature moving forward. In 1.0 we will not automatically generate keys when using shorthand objects or elements. Users will have to pass a key manually. If we add this now, it will encourage users to rely on value being used as the key (at least in the Dropdown only).

If we feel very strongly about fixing this regression, I am willing to ship this as a patch right now to take care of current users as long as we immediately follow it up with a breaking(): ... PR that removes it for 0.69.0.

Yes, I completely agree 👍 Let's ship this fix as 0.68.5 and remove this line in 0.69.0, and mark PR that makes this change as breaking.

codecov-io · 2017-06-01T08:41:09Z

Codecov Report

Merging #1729 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master    #1729   +/-   ##
=======================================
  Coverage   99.75%   99.75%           
=======================================
  Files         142      142           
  Lines        2468     2468           
=======================================
  Hits         2462     2462           
  Misses          6        6

Impacted Files	Coverage Δ
src/modules/Dropdown/Dropdown.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bcc9482...aab7f34. Read the comment docs.

levithomason · 2017-06-01T15:49:53Z

I've left #1729 (comment), I'd like your thoughts there before merging this.

levithomason · 2017-06-01T16:04:15Z

After reading up more on the other issues, I'll update this PR to fix the factory instead. See #1710 (comment).

levithomason · 2017-06-01T18:16:43Z

Released in semantic-ui-react@0.68.5.

* docs(Button): remove redundant prop in Vertical Group example (Semantic-Org#1699) * feat(typescript): Export generics types (Semantic-Org#1698) * docs(Dropdown): fix world icon in search example (Semantic-Org#1695) * Fix MenuExampleText * add world icon to Dropdown example * rename 'languageOptions' to 'languages' * rename languages to languageOptions and separate line into two * separate line of code in two * separate props in different lines * remove trailing spaces * docs(Introduction): fix declarative example (Semantic-Org#1704) * feat(Item): add unstackable prop to ItemGroup (Semantic-Org#1706) * feat(Item): add unstackable prop to ItemGroup * docs(Item): add example for unstackable * chore(package): commit package-lock.json * 0.68.4 * docs(changelog): update changelog [ci skip] * docs(Icon): fix selector for input (Semantic-Org#1714) * chore(package): update require-dir to version 0.3.2 (Semantic-Org#1721) https://greenkeeper.io/ * docs(ItemExampleFloated): your description (Semantic-Org#1719) Fixes error in docs. "AS" is not being recognized. * chore(package): update react-ace to version 5.0.1 (Semantic-Org#1712) https://greenkeeper.io/ * fix(Dropdown): add addition item key (Semantic-Org#1727) * fix(factories): handle falsy `key` values (Semantic-Org#1729) * fix(Dropdown): fix key handling * fix(Dropdown): fix key handling * fix(factories): handle falsy keys * chore(package): update package-lock.json * chore(package): update chai-enzyme to 0.7.1 (Semantic-Org#1731) * feat(typings): expose FormComponent in typings (Semantic-Org#1680) * Exposed form component in typings to support custom controls in packages. * Update index.d.ts * Update index.d.ts * chore(package): update package-lock.json * 0.68.5 * docs(changelog): update changelog [ci skip] * fix(Input): add missing minLength prop (Semantic-Org#1734) * docs(TableExampleSortable): pass in null when that column shouldn't be sorted (Semantic-Org#1737) * feat(TextArea): add minHeight property, docs example (Semantic-Org#1679) * add minHeight property, example to docs * add rows prop/adjust minHeight prop usage * mixed(TextArea): update docs, tests, props and typings * fix(Textarea): move back minHeight prop to style * fix(htmlInputProps): fix handle on falsy values (Semantic-Org#1746) * fix(Search): Allow default action if there is no selected result (Semantic-Org#1742) * docs(images): add missing images, update urls (Semantic-Org#1763) * fix(Accordion): typings inverted to boolean (Semantic-Org#1758) Tiny fix for typings, inverted type incorrectly set to string. * feat(Icon): add ability use the loading prop without an icon (Semantic-Org#1768) * feat(Button): add focus method (Semantic-Org#1764) * fix(Dropdown): change active item on keyboard up/down (Semantic-Org#1735) * fix(Dropdown): change active item on keyboard up/down * fix(Dropdown): change active item on keyboard up/down * refactor(Dropdown): simplify move constant * refactor(Dropdown): remove hidden select (Semantic-Org#1730) breaking(Dropdown): remove hidden select * fix(Checkbox|Input): fix handling of aria-attributes (Semantic-Org#1752) * fix(Checkbox|Input): fix handling of aria-attributes * feat(htmlInputProps): update handling of aria

layershifter added the ready for review label Jun 1, 2017

layershifter commented Jun 1, 2017

View reviewed changes

layershifter force-pushed the fix/dropdown-keys branch from 054bd93 to 8cb5c3b Compare June 1, 2017 08:41

levithomason added needs author feedback and removed ready for review labels Jun 1, 2017

Alexander Fedyashov and others added 4 commits June 1, 2017 09:09

fix(Dropdown): fix key handling

2b2f767

fix(Dropdown): fix key handling

64b89dd

fix(factories): handle falsy keys

f94fed0

chore(package): update package-lock.json

aab7f34

levithomason force-pushed the fix/dropdown-keys branch from 8cb5c3b to aab7f34 Compare June 1, 2017 17:18

levithomason changed the title ~~fix(Dropdown): fix key handling~~ fix(factories): handle falsy key values Jun 1, 2017

levithomason merged commit 2ec4510 into master Jun 1, 2017

levithomason deleted the fix/dropdown-keys branch June 1, 2017 17:29

levithomason mentioned this pull request Jun 1, 2017

breaking(Dropdown): remove hidden select #1730

Merged

noinkling mentioned this pull request Nov 15, 2020

0 or other falsy but non-nullish values being ignored for certain key properties #4111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(factories): handle falsy `key` values #1729

fix(factories): handle falsy `key` values #1729

layershifter commented Jun 1, 2017

layershifter Jun 1, 2017

layershifter Jun 1, 2017

layershifter Jun 1, 2017

levithomason Jun 1, 2017

layershifter Jun 1, 2017

codecov-io commented Jun 1, 2017 •

edited

Loading

levithomason commented Jun 1, 2017

levithomason commented Jun 1, 2017 •

edited

Loading

levithomason commented Jun 1, 2017

fix(factories): handle falsy key values #1729

fix(factories): handle falsy key values #1729

Conversation

layershifter commented Jun 1, 2017

layershifter Jun 1, 2017

Choose a reason for hiding this comment

layershifter Jun 1, 2017

Choose a reason for hiding this comment

layershifter Jun 1, 2017

Choose a reason for hiding this comment

Problem

levithomason Jun 1, 2017

Choose a reason for hiding this comment

layershifter Jun 1, 2017

Choose a reason for hiding this comment

codecov-io commented Jun 1, 2017 • edited Loading

Codecov Report

levithomason commented Jun 1, 2017

levithomason commented Jun 1, 2017 • edited Loading

levithomason commented Jun 1, 2017

fix(factories): handle falsy `key` values #1729

fix(factories): handle falsy `key` values #1729

codecov-io commented Jun 1, 2017 •

edited

Loading

levithomason commented Jun 1, 2017 •

edited

Loading