perf(shorthand): standardize childKey and shorthand props

[levithomason]

INVALID

This PR is being broken down and merged as smaller PRs

This PR standardizes childKey and shorthand props. It also adds commonTests and customPropTypes for these. In doing so, it also defines a custom prop type for the as prop.

Implementing these also means we need to implement the standardized content prop names and patterns for shorthand props. This will also be done here.

Docs / Components

• pull Feed refactors into separate PR

New customPropTypes

• feat(customPropTypes): add "as" prop type #528 as
• childKey
• icon
• image
• shorthand

New commonTests

• feat(commonTests): add implementsShorthandProp #530 implementsShorthandProp

[codecov-io]

Current coverage is 99.63% (diff: 88.88%)

No coverage report found for master at 822ed24.

Powered by Codecov. Last update 822ed24...8dd5a69

[layershifter]

It seems, that we need customProptypes.children.

-children: customPropTypes.every([
-  customPropTypes.disallow(['description', 'header', 'image', 'meta']),
-  PropTypes.node,
-]),
+children: customPropTypes.children(['description', 'header', 'image', 'meta'])

---

I make some updates, see comments.

[layershifter]

Updated all comments on views to unified format.

[levithomason]

Thanks much, I'll pick it up from here.

[levithomason]

I've paused this work. It seems we need to further consider standardization for shorthand props. Thinking out loud...

Initially it seems shorthand props are strings only. Then there are the icon/image exceptions, which can also be a props object or an element (factory). Then there are also two types of array shorthand props, either an array of strings (extraImages, etc) or an array of objects (accordion panels, feed events, breadcrumb sections, etc) which are being handled individually in the render method.

Ideally, there would be a single shorthand prop type and a single shorthand factory for all components.

// Single Usage
<Feed extraText='Hello' />                          // string
<Feed extraText={{ content: 'Hello' }} />           // props
<Feed extraText={<p>Hello</p>} />                   // dom component
<Feed extraText={<Segment>Hello</Segment>} />       // composite component

<Feed date='1 day ago' />                           // string
<Feed date={{ as: 'a', content: '1 day ago' }} />   // props
<Feed date={<img src='1 day ago' />} />             // dom component
<Feed date={<Image src='1 day ago' />} />           // composite component

// Array Usage
<Feed extraImages={['foo.png']} />                  // string
<Feed extraImages={[{ src: 'foo.png' }]} />         // props
<Feed extraImages={[<img src='foo.png' />]} />      // dom component
<Feed extraImages={[<Image src='foo.png' />]} />    // composite component

Shorthand Factory

Could take a Component (string|function), a function mapping shorthand value to Component props, and finally the shorthand value (string|props object|element|array of any of these).

We may also solve #345 reusable component parts by doing this.

// Feed Shorthand Props
const { content, extraImages, extraText, date, meta, summary } = props

const contentJSX = shorthand(FeedContent, x => ({ content: x }), content)
const extraImagesJSX = shorthand(FeedExtra, x => ({ content: x, images: true }), extraImages)
const extraTextJSX = shorthand(FeedExtra, x => ({ content: x, text: true }), extraText)
const dateJSX = shorthand(FeedDate, x => ({ content: x }), date)
const metaJSX = shorthand(FeedMeta, x => ({ content: x }), meta)
const summaryJSX = shorthand(FeedSummary, x => ({ content: x }), summary)

This is almost identical to the createFactory that already exists. We could create a factory for each part (similar to what has been done in #345). Though, it would not handle cases where the array is used to generate more complicated children as in <Accordion panels={} />, <Breadcrumb sections={} />, etc. For this, we would need something more like an actual render function.

customPropTypes.shorthandSingle

String, object, element, disallow children. Intended to be used with simple shorthand props that map directly to sub components (i.e. Feed shorthand).

customPropTypes.shorthandMultiple

Strictly an array of objects. Intended to be used with the more complicated shorthand arrays (Accordion panels, Breadcrumb sections, Feed events, etc).

---

I'm going to think on all this for a bit before continuing.

[layershifter]

If we allow this:

<Feed.Event extraText='Hello' />                          // string
<Feed.Event extraText={{ content: 'Hello' }} />           // props
<Feed.Event extraText={<p>Hello</p>} />                   // dom component
<Feed.Event extraText={<Segment>Hello</Segment>} />       // composite component

We need need allow this:

const items = ['Hello', 'World']
const items = [ {content: 'Hello'}, {content: 'World'} ]
const items = [ <p>Hello</p>, <p>World</p> ]
const items = [ <Segment>Hello</Segment>, <Segment>World</Segment> ]

<Feed.Group items={items} />

Looks cool. Then we can standardize items render:

-  if (children) {
-    return <ElementType {...rest} className={classes}>{children}</ElementType>
-  }

-  const eventsJSX = _.map(events, eventProps => {
-    const { childKey, date, meta, summary, ...eventData } = eventProps
-    const finalKey = childKey || [date, meta, summary].join('-')
-
-    return (
-      <FeedEvent
-        date={date}
-        key={finalKey}
-        meta={meta}
-        summary={summary}
-        {...eventData}
-      />
-    )
-  })
-
-  return <ElementType {...rest} className={classes}>{eventsJSX}</ElementType>
+return (
+  <ElementType {...rest} className={classes}>
+    {children || getItemsJSX(Feed, items)}
+  </ElementType>
+)

And then add commontest for this:

common.implementsItemsProp(Feed)

We can write less code and implementation will be clean and uniform. But, there is only one problem - handling key prop.

[layershifter]

Items

As I said I think that we need standartize items prop, too.

class Menu {
  static PropTypes: customPropTypes.items,

  render() {
    renderItems(Menu, MenuItem, this.props, ['content'])
  }
}

renderItems = (Component, ChildComponent, props, keyProps) => {
  const {children, items} = props

  if(children) {
    return <Component>{children}</Component>
  }

  const itemsJSX = _map(items, item => {
    const {childKey, ...itemProps} = item
    // Ugly variant #1
    const key = childKey || shortid.generate()

    // Ugly variant #2
    const key = () => {
      if(childKey) return childKey

      const keyParts = _map(keyProps, keyProp => {
         const propVal = itemProps[keyProp]

         if(_.isString(propVal)) return propVal
         if(_.isObject(propVal)) return hash(propVal)
      })

      return keyParts.length > 0 ? keyParts.join('-') :  shortid.generate()
   }

    return <ChildComponent {...itemProps} key={key} />
  })
}

@levithomason here is some ugly preudocode. As you might understand, I'm not from frontend, so it may be that there is a decision easier and better, but I'm tried to to suggest an idea 😺

[levithomason]

I'm gonna break this PR up into smaller bite sized PRs.

[layershifter]

Just stupid note about future customPropTypes.shorthand, it cannot be simple PropTypes.node because it includes array.

[jeffcarbs]

@layershifter:

Just stupid note about future customPropTypes.shorthand, it cannot be simple PropTypes.node because it includes array.

I think customPropTypes.shorthand should describe the props for one component. For things like items I think we should just use PropTypes.arrayOf(customPropTypes.shorthand).

Another note, in this PR currently disallow(['children']) is baked into the shorthand prop definition, which I don't think we should do. Take ItemContent for example (not sure if this is a good example):

    <ElementType {...rest} className={classes}>
      {createShorthand(ItemHeader, val => ({ content: val }), header)}
      {createShorthand(ItemMeta, val => ({ content: val }), meta)}
      {createShorthand(ItemDescription, val => ({ content: val }), description)}
      {createShorthand(ItemExtra, val => ({ content: val }), extra)}
      {children || content}
    </ElementType>

We're using shorthand for header, meta, description, extra, but also allowing children, which I think is a totally valid use-case.

[layershifter]

@jcarbo:

For things like items I think we should just use PropTypes.arrayOf(customPropTypes.shorthand).

We need also check childKey, so it might be more complex.

[levithomason]

I think we should just use PropTypes.arrayOf(customPropTypes.shorthand.

👍

We need also check childKey, so it might be more complex.

There is now a function (#543) that deals with merging the key. It can also deal with creating one. We can use the string/number literal if provided, this was one of the reasons we limited shorthand to strings/numbers. If the user provides a props object or an element, then they can also pass a key or childKey themselves, which is reasonable. I think this covers all the bases

disallow(['children']) is baked into the shorthand prop definition, which I don't think we should do.

There are deep seated reasons why we should not allow children with shorthand. The CONTRIBUTING.md guide for component parts points this out and links this response as the thorough reasoning. Though I want to allow it, I'm still of the mind for v1 that is best not to for the reasons noted.

[levithomason]

What is mostly remaining here is Feed component prop updates and examples. I'll make a PR pulling those out soon.

[levithomason]

With the merge of #618 this work is done. Closing.