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

* fix(Dropdown): fix key handling * fix(Dropdown): fix key handling * fix(factories): handle falsy keys * chore(package): update package-lock.json
Semantic-Org · Jun 1, 2017 · 2ec4510 · 2ec4510
1 parent bcc9482
commit 2ec4510
Show file tree

Hide file tree

Showing 5 changed files with 127 additions and 41 deletions.
diff --git a/package-lock.json b/package-lock.json
diff --git a/src/lib/factories.js b/src/lib/factories.js
@@ -77,10 +77,10 @@ export function createShorthand(Component, mapValueToProps, val, options = {}) {
   // ----------------------------------------
 
   // Use key, childKey, or generate key
-  if (!props.key) {
+  if (_.isNil(props.key)) {
     const { childKey } = props
 
-    if (childKey) {
+    if (!_.isNil(childKey)) {
       // apply and consume the childKey
       props.key = typeof childKey === 'function' ? childKey(props) : childKey
       delete props.childKey

diff --git a/src/modules/Dropdown/Dropdown.js b/src/modules/Dropdown/Dropdown.js
@@ -25,6 +25,8 @@ import DropdownMenu from './DropdownMenu'
 
 const debug = makeDebugger('dropdown')
 
+const getKeyOrValue = (key, value) => _.isNil(key) ? value : key
+
 /**
  * A dropdown allows a user to select a value from a series of options.
  * @see Form
@@ -1072,7 +1074,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>
         ))}
       </select>
     )
@@ -1137,7 +1139,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),
         onClick: this.handleLabelClick,
         onRemove: this.handleLabelRemove,
         value: item.value,
@@ -1168,6 +1170,7 @@ export default class Dropdown extends Component {
       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' },
     }))

diff --git a/test/specs/lib/factories-test.js b/test/specs/lib/factories-test.js
@@ -168,55 +168,119 @@ describe('factories', () => {
       })
     })
 
-    describe('child key', () => {
-      it('uses the `key` prop from an element', () => {
-        getShorthand({ value: <div key='foo' /> })
-          .should.have.property('key', 'foo')
+    describe('key', () => {
+      it('is not consumed', () => {
+        getShorthand({ value: { key: 123 } })
+          .props.should.have.property('key')
       })
 
-      it('uses the `key` prop as a string', () => {
-        getShorthand({ value: { key: 'foo' } })
-          .should.have.property('key', 'foo')
-      })
+      describe('on an element', () => {
+        it('works with a string', () => {
+          getShorthand({ value: <div key='foo' /> })
+            .should.have.property('key', 'foo')
+        })
 
-      it('uses the `key` prop as a number', () => {
-        getShorthand({ value: { key: 123 } })
-          .should.have.property('key', '123')
+        it('works with a number', () => {
+          getShorthand({ value: <div key={123} /> })
+            .should.have.property('key', '123')
+        })
+
+        it('works with falsy values', () => {
+          getShorthand({ value: <div key={null} /> })
+            .should.have.property('key', 'null')
+
+          getShorthand({ value: <div key={0} /> })
+            .should.have.property('key', '0')
+
+          getShorthand({ value: <div key='' /> })
+            .should.have.property('key', '')
+        })
       })
 
-      it('uses the `childKey` prop as a string', () => {
-        getShorthand({ value: { childKey: 'foo' } })
-          .should.have.property('key', 'foo')
+      describe('on an object', () => {
+        it('works with a string', () => {
+          getShorthand({ value: { key: 'foo' } })
+            .should.have.property('key', 'foo')
+        })
+
+        it('works with a number', () => {
+          getShorthand({ value: { key: 123 } })
+            .should.have.property('key', '123')
+        })
+
+        it('works with falsy values', () => {
+          getShorthand({ value: { key: null } })
+            .should.have.property('key', 'null')
+
+          getShorthand({ value: { key: 0 } })
+            .should.have.property('key', '0')
+
+          getShorthand({ value: { key: '' } })
+            .should.have.property('key', '')
+        })
       })
+    })
 
-      it('uses the `childKey` prop as a number', () => {
+    describe('childKey', () => {
+      it('is consumed', () => {
         getShorthand({ value: { childKey: 123 } })
-          .should.have.property('key', '123')
+          .props.should.not.have.property('childKey')
       })
 
-      it('calls `childKey` with the final `props` if it is a function', () => {
-        const props = { foo: 'foo', childKey: sandbox.spy(({ foo }) => foo) }
+      it('is called with the final `props` if it is a function', () => {
+        const props = { foo: 'bar', childKey: sandbox.spy(({ foo }) => foo) }
         const element = getShorthand({ value: props })
 
         props.childKey.should.have.been.calledOnce()
-        props.childKey.should.have.been.calledWithExactly({ foo: 'foo', key: 'foo' })
+        props.childKey.should.have.been.calledWithExactly({ foo: 'bar', key: 'bar' })
 
-        element.key.should.equal('foo')
+        element.key.should.equal('bar')
       })
 
-      it('consumes the childKey prop', () => {
-        getShorthand({ value: { childKey: 123 } })
-          .props.should.not.have.property('childKey')
-      })
+      describe('on an element', () => {
+        it('works with a string', () => {
+          getShorthand({ value: <div childKey='foo' /> })
+            .should.have.property('key', 'foo')
+        })
+
+        it('works with a number', () => {
+          getShorthand({ value: <div childKey={123} /> })
+            .should.have.property('key', '123')
+        })
+
+        it('works with falsy values', () => {
+          getShorthand({ value: <div childKey={null} /> })
+            .should.have.property('key', null)
 
-      it('is generated from shorthand string values', () => {
-        getShorthand({ value: 'foo' })
-          .should.have.property('key', 'foo')
+          getShorthand({ value: <div childKey={0} /> })
+            .should.have.property('key', '0')
+
+          getShorthand({ value: <div childKey='' /> })
+            .should.have.property('key', '')
+        })
       })
 
-      it('is generated from shorthand number values', () => {
-        getShorthand({ value: 123 })
-          .should.have.property('key', '123')
+      describe('on an object', () => {
+        it('works with a string', () => {
+          getShorthand({ value: { childKey: 'foo' } })
+            .should.have.property('key', 'foo')
+        })
+
+        it('works with a number', () => {
+          getShorthand({ value: { childKey: 123 } })
+            .should.have.property('key', '123')
+        })
+
+        it('works with falsy values', () => {
+          getShorthand({ value: { childKey: null } })
+            .should.have.property('key', null)
+
+          getShorthand({ value: { childKey: 0 } })
+            .should.have.property('key', '0')
+
+          getShorthand({ value: { childKey: '' } })
+            .should.have.property('key', '')
+        })
       })
     })
 

diff --git a/test/specs/modules/Dropdown/Dropdown-test.js b/test/specs/modules/Dropdown/Dropdown-test.js
@@ -1497,6 +1497,20 @@ describe('Dropdown', () => {
         .find('DropdownItem')
         .everyWhere(item => item.should.have.prop('data-foo', 'someValue'))
     })
+
+    it('handles keys correctly', () => {
+      const customOptions = [
+        { key: 0, text: 'foo', value: 'foo' },
+        { key: null, text: 'bar', value: 'bar' },
+        { key: undefined, text: 'baz', value: 'baz' },
+      ]
+      wrapperShallow(<Dropdown options={customOptions} selection />)
+      const items = wrapper.find('DropdownItem')
+
+      items.at(0).key().should.equal('0')
+      items.at(1).key().should.equal('bar')
+      items.at(2).key().should.equal('baz')
+    })
   })
 
   describe('selection', () => {