Dropdown: option keys fallback to values incorrectly

[notgiorgi]

Steps

if we define our Dropdown component like this:

const opts = [
  {
    key: 0,
    value: 1,
    text: 'Foo',
  },
  {
    key: 1,
    value: 2,
    text: 'Bar',
  },
]

const MyDropdown = () => <Dropdown selection options={opts} />

Note: this only happens with selection prop true, because only in that case, select is rendered behind the scenes

Expected Result

This should render <select> with two <option>s with keys 0 and 1

Actual Result

This tries to render <select> with two <option>s with keys 1 and 1 because of { option.key || option.value } (see this line of the code https://github.com/Semantic-Org/Semantic-UI-React/blob/master/src/modules/Dropdown/Dropdown.js#L1074)

I think this line should be rewritten to only fallback to option.value when option.key is either undefined or empty string. All other falsy values are valid keys, react converts them to strings.

Version

last

Testcase

https://codepen.io/notgiorgi/pen/ZKgBob?editors=0010 (you cannot use devtools on codepen)

[notgiorgi]

One approach can be to write a helper function:

function getValidKey(key) {
  switch (key) {
    case null: return 'null'
    case false: return 'false'
    case 0: return '0'

    case undefined:
    case '':
      return false
    default: return key
  }
}

and then

key={getValidKey(option.key) || option.value}

Or we could do

function getValidKey(key, fallback) {
  switch (key) {
    case undefined:
    case '':
      return fallback
    default: return key
  }
}

and then

key={getValidKey(option.key, option.value)}

I can send a PR

[layershifter]

@notgiorgi Nice catch, there is also #1710. I think that both issues should be fixed in a single PR.

[notgiorgi]

I can do them both, which solution should I go with? I mentioned two above.

[layershifter]

I don't like idea with getValidKey because key handling is responsibility of our factories. However, it means a large refactoring like #1619, not time for this.

getValidKey

I think it will be enought to use lodash's isNil (check to null and undefined):

const getKeyOrVal = (key, value) => _.isNil(key) ? value : key

And it should be placed directly in Dropdown.js because as I said this pattern shouldn't reused in other components.

These lines should be updated:

• L1074
• L1139

update factory's usage

This will fix #1710. I commented why this problem occurs, fix for it is pretty simple:

    return _.map(options, (opt, i) => DropdownItem.create({
      active: isActive(opt.value),
      onClick: this.handleItemClick,
      selected: selectedIndex === i,
      ...opt,
+     key: getKeyOrValue(opt.key, opt.value),
      // Needed for handling click events on disabled items
      style: { ...opt.style, pointerEvents: 'all' },
    }))

---

@notgiorgi I'll be glad to see PR that makes these changes.

[notgiorgi]

I was gonna say, I'll get into it on weekend, but it seems to be done already 😄

[levithomason]

Yes sir, cutting a release as we speak.

[levithomason]

Released in [email protected].