feat(common): Improve shorthand array handling

[jeffcarbs]

Note: Abstracted from #430

---

This updates the createShorthand factory to take two special props as extraProps to generate a key if missing. This is necessary for shorthand props that are arrays. The props are:

• childKey - enables explicitly setting the key via props
• getChildKey - function that takes the given props and returns a key

This PR 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 which are valid shorthand.
• You should be able to reliably set the activeIndex or defaultActiveIndex via props, but this would overwrite that on mount.

---

I was also thinking about just using childKey that could either be a function or a string, so:

const mergeChildKey = ({ childKey, ...props }) => {
  if (!props.key) {
    props.key = _.isFunction(childKey) ? childKey(props) : childKey
  }

  return props
}

But then we'd have to make sure that if the childKey were passed in props, it's not overwritten by extraProps. Might be worth it to remove the need for that additional key.

[jeffcarbs]

Could also simplify to:

const mergePropsAndClassName = (props, extraProps) => {
  const { childKey, ...newProps } = { ...props, ...extraProps }

  if (_.has(props, 'className') || _.has(extraProps.className)) {
    newProps.className = cx(props.className, extraProps.className) // eslint-disable-line react/prop-types
  }

  if (!props.key) {
    props.key = _.isFunction(childKey) ? childKey(props) : childKey
  }

  return mergeChildKey(newProps)
}

This also makes me wonder, currently anything in extraProps will "win" out over props in the merge. Should this be the case? I know generally extraProps are things necessary for SUI functionality (like active), but I'd imagine we'd want to give the user the ultimate control. Thoughts?

[levithomason]

I was also thinking about just using childKey that could either be a function or a string, so:

Love the function idea. Unrelated to this PR, I was thinking the extraProps should have been a function as well. It is sometimes necessary to build up extra props based on the original factory value/props/returned element.

This PR also fixes some issues I noticed using the menu items shorthand prop:

👍

Regarding comments on extraProps overriding the props, I think the override is mixed up actually (changed my response as I wrote it!). There are 3 levels of props at play:

1. mapValueToProps

   The base props object that the factory defines from a shorthand value.

2. extraProps

   Passed to the factory when called createIcon('user', { color: 'red' }). I think of these sort of like default props. They are only should only be applicable if the user does not provide the same props.

3. User's props

   This is where it is mixed up. The extraProps should only override when the factory value is a literal. That makes sense because the user's props should always win, IMO. So, if the user passes a props object or an element then we want to use their props object/element props as the extraProps. This way, they can override the factory's "base props" and "extra props" defined in the factory call.

Simply flipping the args in those calls to mergePropsAndClassName() will ensure the user's props (if provided) always win. It may then also make sense to rename them from props extraProps to defaultProps props so it is more clear why one is winning over the other.

[codecov-io]

Current coverage is 98.86% (diff: 100%)

Merging #543 into master will decrease coverage by <.01%

@@             master       #543   diff @@
==========================================
  Files           108        108          
  Lines          1764       1762     -2   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           1744       1742     -2   
  Misses           20         20          
  Partials          0          0          

Powered by Codecov. Last update 650d1d8...bc5d81f

[jeffcarbs]

@levithomason - Updated based on the comments here.

• Just going to use childKey which could be either a literal or a function.
• Got rid of the separate method and just handle childKey as part of the mergePropsAndClassName function.
• Swapped and renamed extraProps to defaultProps.

I think should be g2g 👍

[levithomason]

Just one comment regarding defaultProps ordering

[levithomason]

In this case, I believe the factory's mapValueToProps is the default. The other props here are what ever the developer passed when calling createFoo. Otherwise, you wouldn't be able to override the default mapping when using the component.

Feedback?

[jeffcarbs]

The defaultProps will always be what we (the stardust authors) set as the default. The props, whether passed directly or derived from mapValueToProps, will be what the end-user wants. So those should always win.

Take this example component that has an image with a default src:

// Example:
const Foo = (props) => (
  <div>
    createShorthand(Image, val => ({ src: val }), props.image, { src: 'foo.png' })
  </div>
)

The different usages are:

React Element:

<Foo image={<MyImage src='bar.png'> />

// Within createShorthand:
// Clone ReactElements
if (isValidElement(val)) {
  return React.cloneElement(val, mergePropsAndClassName(defaultProps, val.props))
}

Props object

<Foo image={{ src: 'bar.png' }} />

// Within createShorthand:
// Create ReactElements from props objects
if (_.isPlainObject(val)) {
  return <Component {...mergePropsAndClassName(defaultProps, val)} />
}

Primitive:

<Foo image='bar.png' />

// Within createShorthand:
// Map values to props and create a ReactElement
if (_.isString(val) || _.isNumber(val)) {
  return <Component {...mergePropsAndClassName(defaultProps, mapValueToProps(val))} />
}

In all of these cases, if we swapped the argument order of defaultProps the src would not be what the user passed.

[levithomason]

Wrote out a detailed explanation of how I didn't actually think this would work for the string/number literal case only to come to see it correctly. Thanks for the clarifications!

[levithomason]

Released in stardust@0.46.3, thanks!