feat(common): Improve shorthand array handling

This updates the createShorthand factory to handle setting key
- If the key is set, leaves it as-is
- If the childKey prop is a string/number it uses childKey as the key
- If the childKey prop is a function it called childKey with props and uses the
return value as the key.

This also fixes some issues I noticed using the menu `items` shorthand prop:
- Use the createShorthand factory to create a MenuItem for each item.
- Do not infer activeIndex from the items array on mount.

Regarding the second point, there were a few issues with it:
- The array items may not be objects - they could be primitives or elements
- You should be able to reliably set the activeIndex or defaultActiveIndex
via props, but this would overwrite that.

src/collections/Menu/Menu.js

@@ -14,6 +14,7 @@ import {
   useValueAndKey,
   useWidthProp,
 } from '../../lib'
+import { createShorthand } from '../../factories'
 import MenuHeader from './MenuHeader'
 import MenuItem from './MenuItem'
 import MenuMenu from './MenuMenu'
@@ -92,14 +93,12 @@ class Menu extends Component {
     /** Shorthand array of props for Menu. Mutually exclusive with children. */
     items: customPropTypes.every([
       customPropTypes.disallow(['children']),
-      PropTypes.arrayOf(PropTypes.shape({
-        childKey: PropTypes.oneOfType([
-          PropTypes.number,
-          PropTypes.string,
-        ]),
-        // this object is spread on the MenuItem
-        // allow it to validate props instead
-      })),
+      // Array of shorthands for MenuItem
+      PropTypes.arrayOf(PropTypes.oneOfType([
+        PropTypes.string,
+        PropTypes.element,
+        PropTypes.object,
+      ])),
     ]),
 
     /** onClick handler for MenuItem. Mutually exclusive with children. */
@@ -149,13 +148,6 @@ class Menu extends Component {
   static Item = MenuItem
   static Menu = MenuMenu
 
-  componentWillMount() {
-    super.componentWillMount()
-
-    const { items } = this.props
-    if (items) this.trySetState({ activeIndex: _.findIndex(items, ['active', true]) })
-  }
-
   handleItemClick = (e, { name, index }) => {
     this.trySetState({ activeIndex: index })
     const { items, onItemClick } = this.props
@@ -169,20 +161,12 @@ class Menu extends Component {
     const { activeIndex } = this.state
 
     return _.map(items, (item, index) => {
-      const { content, childKey, name, itemProps } = item
-      const finalKey = childKey || [content, name].join('-')
-
-      return (
-        <MenuItem
-          {...itemProps}
-          active={activeIndex === index}
-          content={content}
-          index={index}
-          key={finalKey}
-          name={name}
-          onClick={this.handleItemClick}
-        />
-      )
+      return createShorthand(MenuItem, val => ({ content: val }), item, {
+        active: activeIndex === index,
+        childKey: ({ content, name }) => [content, name].join('-'),
+        index,
+        onClick: this.handleItemClick,
+      })
     })
   }
 

src/factories.js

@@ -9,15 +9,19 @@ import Label from './elements/Label/Label'
 /**
  * Merges props and classNames.
  *
+ * @param {object} defaultProps A props object
  * @param {object} props A props object
- * @param {object} extraProps A props object
  * @returns {object} A new props object
  */
-const mergePropsAndClassName = (props, extraProps) => {
-  const newProps = { ...props, ...extraProps }
+const mergePropsAndClassName = (defaultProps, props) => {
+  const { childKey, ...newProps } = { ...defaultProps, ...props }
 
-  if (_.has(props, 'className') || _.has(extraProps.className)) {
-    newProps.className = cx(props.className, extraProps.className) // eslint-disable-line react/prop-types
+  if (_.has(props, 'className') || _.has(defaultProps.className)) {
+    newProps.className = cx(defaultProps.className, props.className) // eslint-disable-line react/prop-types
+  }
+
+  if (!newProps.key) {
+    newProps.key = _.isFunction(childKey) ? childKey(newProps) : childKey
   }
 
   return newProps
@@ -30,23 +34,23 @@ const mergePropsAndClassName = (props, extraProps) => {
  * @param {function|string} Component A ReactClass or string
  * @param {function} mapValueToProps A function that maps a primitive value to the Component props
  * @param {string|object|function} val The value to create a ReactElement from
- * @param {object} extraProps Additional props to add to the final ReactElement
+ * @param {object} defaultProps Default props to add to the final ReactElement
  * @returns {function|null}
  */
-export function createShorthand(Component, mapValueToProps, val, extraProps = {}) {
+export function createShorthand(Component, mapValueToProps, val, defaultProps = {}) {
   // Clone ReactElements
   if (isValidElement(val)) {
-    return React.cloneElement(val, mergePropsAndClassName(val.props, extraProps))
+    return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
   }
 
   // Create ReactElements from props objects
   if (_.isPlainObject(val)) {
-    return <Component {...mergePropsAndClassName(val, extraProps)} />
+    return <Component {...mergePropsAndClassName(defaultProps, val)} />
   }
 
   // Map values to props and create a ReactElement
   if (_.isString(val) || _.isNumber(val)) {
-    return <Component {...mergePropsAndClassName(mapValueToProps(val), extraProps)} />
+    return <Component {...mergePropsAndClassName(defaultProps, mapValueToProps(val))} />
   }
 
   // Otherwise null

test/specs/collections/Menu/Menu-test.js

@@ -1,3 +1,4 @@
+import _ from 'lodash'
 import React from 'react'
 
 import Menu from 'src/collections/Menu/Menu'
@@ -39,30 +40,33 @@ describe('Menu', () => {
   })
 
   describe('activeIndex', () => {
-    it('-1 by default', () => {
-      const items = [
-        { name: 'home' },
-        { name: 'users' },
-      ]
+    const items = [
+      { name: 'home' },
+      { name: 'users' },
+    ]
 
-      shallow(<Menu items={items} />).should.have.state('activeIndex', -1)
+    it('is null by default', () => {
+      shallow(<Menu items={items} />)
+        .should.not.have.descendants('.active')
     })
 
-    it('can be overridden with "active"', () => {
-      const items = [
-        { name: 'home' },
-        { active: true, name: 'users' },
-      ]
+    it('is set when clicking an item', () => {
+      // random item, skip the first as its selected by default
+      const randomIndex = _.random(items.length - 1)
 
-      shallow(<Menu items={items} />).should.have.state('activeIndex', 1)
+      mount(<Menu items={items} />)
+        .find('MenuItem')
+        .at(randomIndex)
+        .simulate('click')
+        .should.have.prop('active', true)
     })
   })
 
   describe('items', () => {
     const spy = sandbox.spy()
     const items = [
-      { name: 'home', onClick: spy },
-      { active: true, name: 'users' },
+      { name: 'home', onClick: spy, 'data-foo': 'something' },
+      { name: 'users', active: true, 'data-foo': 'something' },
     ]
     const children = mount(<Menu items={items} />).find('MenuItem')
 
@@ -85,6 +89,10 @@ describe('Menu', () => {
       spy.should.have.been.calledOnce()
       spy.should.have.been.calledWithMatch(event, props)
     })
+
+    it('passes arbitraty props', () => {
+      children.everyWhere(item => item.should.have.prop('data-foo', 'something'))
+    })
   })
 
   describe('onItemClick', () => {