feat(shorthand): Custom shorthand proptypes #618

jeffcarbs · 2016-10-04T07:43:29Z

This adds a few new customPropTypes (took some code and ideas from #452, thanks @levithomason 👍). It's very much WIP but wanted to get it up and get some feedback before I go too crazy here. The new propTypes are:

itemShorthand
- disallows children
- one of: string, number, object, element
arrayOfShorthand (better name?)
- disallows children
- arrayOf(itemShorthand)
collectionShorthand
- disallows children
- one of: arrayOfShorthand, object, element
nodeShorthand
- disallows children
- node

The large majority of our components have a content prop that can be rendered in place of the children. This was validated differently in various components as strings/numbers/elements, but really it can be any renderable thing. This is where the nodeShorthand came from. We could theoretically just use node but I wanted to be able to disallow children.

I also wound up simplifying/standardizing the doc blocks for common propTypes (there was like 10 ways we described the className prop haha). I did it via regex so it's super easy to change so let me know if you think the descriptions could be improved.

Open questions:

We double-disallow right now, e.g. content disallows children, children disallows content. Is there anything wrong with only disallowing on one side? Assuming all shorthands disallow children, children could just be PropTypes.node which is definitely simpler. Currently it's a customPropType that disallows any shorthands (inferring which are shorthand props from Component.propTypes)

cc @levithomason @layershifter

codecov-io · 2016-10-04T07:46:51Z

Current coverage is 100% (diff: 100%)

Merging #618 into master will not change coverage

@@           master   #618   diff @@
====================================
  Files         119    119          
  Lines        1881   1887     +6   
  Methods         0      0          
  Messages        0      0          
  Branches        0      0          
====================================
+ Hits         1881   1887     +6   
  Misses          0      0          
  Partials        0      0

Powered by Codecov. Last update ddb6349...9928ccc

levithomason · 2016-10-04T07:48:38Z

Ah! So pumped to see this :D I'm in agreement with everything noted in this PR. These exact items were on my bucket list.

Is there anything wrong with only disallowing on one side? Assuming all shorthands disallow children, children could just be PropTypes.node which is definitely simpler.

Yes please! One way disallow is perfect, children can just be node.

Icon/Image

I also had an icon and image shorthand prop type that disallowed the other, #452. That way you can only add an icon or an image but not both. This is applicable in places like Label and Header. Can't recall where else if any.

layershifter · 2016-10-04T07:56:19Z

May be some shorter names?

itemShorthand => item
arrayOfShorthand => items
collectionShorthand => collection
nodeShorthand => content (nodeShorthand only used on content prop as I see)

It will look good with icon and image.

I also wound up simplifying/standardizing the doc blocks for common propTypes

👍

levithomason · 2016-10-04T07:49:35Z

.github/CONTRIBUTING.md

@@ -490,7 +490,7 @@ Label.propTypes = {
  /** A label can reduce its complexity. */
  basic: PropTypes.bool,

-  /** Primary content of the label, same as text. */
+  /** Primary content. */
  children: PropTypes.node,

  /** Classes to add to the label className. */


Any chance we could also grep the className doc block? Just about every one is different as well :/

EDIT I see many others were caught, perhaps just add this one?

levithomason · 2016-10-04T07:52:25Z

src/collections/Table/Table.js

@@ -83,7 +83,7 @@ function Table(props) {
  return (
    <ElementType {...rest} className={classes}>
      {headerRow && <TableHeader>{TableRow.create(headerRow, { cellAs: 'th' })}</TableHeader>}
-      {<TableBody>{_.map(tableData, (data, index) => TableRow.create(renderBodyRow(data, index)))}</TableBody>}
+      <TableBody>{_.map(tableData, (data, index) => TableRow.create(renderBodyRow(data, index)))}</TableBody>


Seems we should be checking for renderBodyRow before calling it:

{renderBodyRow && <TableBody ...}

Otherwise, it's gonna throw if the user only gives a header/footer config.

levithomason · 2016-10-04T08:02:21Z

src/lib/customPropTypes.js

+ */
+export const children = (Component) => (...args) => {
+  const disallowedProps = _.flow(
+    _.pickBy(checker => _.some(shorthandPropTypes, (propType) => propType === checker)),


Is this working correctly? This file uses lodash/fp which takes data last, so I'd expect the shorthandPropTypes (data) to be provided after the some function.

Also, might be slightly cleaner to use _.includes:

_.pickBy(checker => _.includes(checker, shorthandPropTypes))

I believe this is correct, data last again. Didn't test it though.

As per https://github.com/TechnologyAdvice/stardust/pull/618#issuecomment-251318050 I'm just going to scrap this whole custom propType and make all the children propType.node. The "disallow" will happen on the shorthand side.

levithomason · 2016-10-04T08:08:34Z

Can we also close #452 with this PR? It pretty much covers the rest of that one. The only remaining prop of concern is childKey, and that will get handled when do the collection factory I'm sure.

jeffcarbs · 2016-10-04T16:56:52Z

Apologies for the long PR, but I don't think there's a great way to break it up and most of the changed lines are just comments. FWIW, I ran specs before/after and there were the same number of warnings, so I'm pretty confident this didn't introduce any regressions in prop validations.

This is ready for review now, but let's hold off on merging until #619 since there are going to be a ton of merge conflicts.

TODO on separate PRs:

There are some components that were too complicated to try to unwind so I didn't apply the custom prop types there. Dropdown and Button are the main two. I think there's work to be done to try to simplify the logic there, if only to make it more clear to us what's going on.
Audit components that are rendering children + shorthand (e.g. DropdownItem, Message, FeedEvent) and see if we can refactor to avoid it.
We currently have a factory function that convertscustomPropTypes.item => Component. It would be nice to also have one that converts customPropTypes.items => [Component, Component, ...]. In some cases this is as simple as _.map(items, SubComponent.create) but many times the props depend on the index (e.g. active = selectedIndex === index), so TBD there.

levithomason · 2016-10-04T20:55:20Z

Bump, this is ready for conflict resolutions. Good luck my friend 🎱

jeffcarbs · 2016-10-04T21:35:50Z

@levithomason - Already rebased and handled the conflicts 😄 This is ready for final review 👍

levithomason · 2016-10-04T22:37:23Z

Oh snap! I'm on it, wanna trade me reviews? #552 factory tests :)

levithomason

Accepting as I only have 2 suggestions that I'm deferring to you on. Will merge on your word, cheers!

levithomason · 2016-10-04T22:41:16Z

src/lib/customPropTypes.js

+ * ArrayOf shorthand ensures a prop is an array of item shorthand
+ */
+export const items = (...args) => every([
+  disallow(['children']),


This is duplicate disallowing since the item disallows. Suggest removing that. I'm also not sure we should have this checker. Seems we could just do PropTypes.arrayOf(item) in the component and collection below to simplify. Will let you make the call on that.

I'm trying to unwind this a little bit because it's gets a little complicated with the nested every calls. Turns out this disallow is actually necessary, I'll try to explain. Consider the following:

const cells = ['Name', 'Status', 'Notes'] <TableRow cells={cells}>some child</TableRow>

If cells were PropTypes.arrayOf(item) this doesn't give a warning because arrayOf will use the given validation (item in this case) to validate each item in the cells within the context of cells, not within the context of TableRow.

Nested disallow doesn't actually seem to work as expected since it operates on the parent context of the given prop. So when checking cells with:

const items = (...args) => every([ disallow(['children']), PropTypes.arrayOf(item), ])(...args)

It's evaluating cells within the context of the TableRow props and asking "is cells an array of items AND is the children key undefined in TableRow props". When it's evaluating each item via arrayOf(item), it's asking "is this item one of these valid types AND is the children key undefined in this item's array" (i.e. array['children']) which doesn't throw but doesn't really make sense

I hope that makes sense but it's pretty complicated haha.

Got it, perhaps that is one of the differences with the arrayOf checker. I know if has a different signature and operates uniquely compared to all other checkers. Either way, it makes sense and I think we leave it as is for now.

levithomason · 2016-10-04T22:43:37Z

src/lib/customPropTypes.js

@@ -181,6 +181,49 @@ export const demand = (requiredProps) => {
 }

 /**
+ * Ensure a component can render as a node passed as a prop value in place of children.
+ */
+export const content = (...args) => every([


Apologies for the lack of response when you asked about this previously, but I really liked having the longer names here. I think it is more clear and more in line with the React community to be explicit. Especially being collection is a legitimate data type and the collection propType refers to something different, shorthand.

contentShorthand itemShorthand collectionShorthand

Given all the back and forth you suffered here, if you wanna push this off to another PR, I get that and am OK doing so. :)

No worries, I don't think there's been much back and forth here.

I'm torn because I like the terseness of the shorter names but I'm also kinda leaning these longer names. Ultimately I think clarity wins out, I'll update the names now.

jeffcarbs · 2016-10-05T01:52:04Z

See that last comment regarding the need for the four types. I'm going to rename the others, but I'm struggling to think of a clear naming distinction between the items and collection. In this case, a "collection" is an item/entity that has a set of "items" and "items" is the array of items. However, arrays are commonly referred to as "collections" so it's kinda confusing.

What are your thoughts on adding array as a valid type of itemShorthand? An array is technically a valid "node" in that it's renderable wherever a string/number/element is renderable. I could see a user wanting to pass sibling elements as shorthand without a wrapping div (I actually did that in search: https://github.com/TechnologyAdvice/stardust/blob/master/src/modules/Search/SearchResult.js#L18-L27)

It could be up to the the component's create function how it wanted to handle a given literal. For example when passing non-arrays to TableRow.create it could either cast it to an array or throw. This also lines up with the createShorthand factory which treats string/number/array the same. It's also possibly more consistent since TableRow.create(items) is technically creating an item, not a collection/array.

This would mean that there'd be the following prop validations:

contentShorthand - node/element
itemShorthand - array/number/string/props-object/element
collectionShorthand - arrayOf(itemShorthand)

Hate to throw a curveball like this after a PR has gone through review, but my gut is telling me maybe just those three would be sufficient and we can split out a itemWithCollection proptype later.

Thoughts?

jeffcarbs · 2016-10-05T03:13:48Z

Appended Shorthand to all of the current shorthand prop types for now. LMK what you think about my last comment re: collapsing what is now collectionShorthand into itemShorthand and using collectionShorthand for the arrayOf(itemShorthand) like items and cells.

levithomason · 2016-10-05T15:41:15Z

This would mean that there'd be the following prop validations:

contentShorthand - node/element

itemShorthand - array/number/string/props-object/element

collectionShorthand - arrayOf(itemShorthand)

This seems correct to me, since it mirrors the createShorthand factory's supported types.

I also note that since we've expanded it's ability and definition quite a bit, itemShorthand is actually node/object now. Since node includes numbers, strings, elements and array (of any of these). That being the case, contentShorthand is also just node since node includes elements. So, I think we're left with:

contentShorthand - node
itemShorthand - node/object
collectionShorthand - arrayOf(itemShorthand)

Where we're actually just adding object to the already valid renderable React types.

jeffcarbs · 2016-10-05T16:30:17Z

👍 just made the updates. Once the specs pass I think we're cool to merge

levithomason · 2016-10-05T16:32:08Z

Sounds good, will do.

jeffcarbs force-pushed the feature/shorthand-proptype branch from c5ec8e3 to 40a40ed Compare October 4, 2016 07:46

levithomason requested changes Oct 4, 2016

View reviewed changes

jeffcarbs force-pushed the feature/shorthand-proptype branch from 40a40ed to 0ff9c3a Compare October 4, 2016 16:39

jeffcarbs added the ready for review label Oct 4, 2016

levithomason mentioned this pull request Oct 4, 2016

refactor(Components): use a class if there are event handlers #619

Merged

9 tasks

levithomason added needs author feedback and removed ready for review labels Oct 4, 2016

jeffcarbs force-pushed the feature/shorthand-proptype branch from 0ff9c3a to 23803e0 Compare October 4, 2016 20:57

jeffcarbs added ready for review and removed needs author feedback labels Oct 4, 2016

levithomason approved these changes Oct 4, 2016

View reviewed changes

levithomason added needs author feedback and removed ready for review labels Oct 4, 2016

jeffcarbs added RFC and removed needs author feedback labels Oct 5, 2016

jeffcarbs force-pushed the feature/shorthand-proptype branch from 23803e0 to 2290392 Compare October 5, 2016 03:10

jeffcarbs added ready for review and removed RFC labels Oct 5, 2016

levithomason added the needs author feedback label Oct 5, 2016

levithomason removed the ready for review label Oct 5, 2016

jeffcarbs added 2 commits October 5, 2016 12:22

feat(shorthand): Custom shorthand proptypes

df0b6ce

Rename shorthand proptypes

9928ccc

jeffcarbs force-pushed the feature/shorthand-proptype branch from 2290392 to 9928ccc Compare October 5, 2016 16:26

levithomason added ready for review and removed needs author feedback labels Oct 5, 2016

levithomason merged commit 6825109 into master Oct 5, 2016

levithomason deleted the feature/shorthand-proptype branch October 5, 2016 19:16

This was referenced Oct 6, 2016

fix(customPropTypes): itemsShorthand -> collectionShorthand #632

Merged

perf(shorthand): standardize childKey and shorthand props #452

Closed

layershifter mentioned this pull request Oct 6, 2016

Standardize childKey and shorthand props #449

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(shorthand): Custom shorthand proptypes #618

feat(shorthand): Custom shorthand proptypes #618

jeffcarbs commented Oct 4, 2016 •

edited

Loading

codecov-io commented Oct 4, 2016 •

edited

Loading

levithomason commented Oct 4, 2016

layershifter commented Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016

jeffcarbs Oct 4, 2016

levithomason commented Oct 4, 2016

jeffcarbs commented Oct 4, 2016 •

edited

Loading

levithomason commented Oct 4, 2016

jeffcarbs commented Oct 4, 2016

levithomason commented Oct 4, 2016

levithomason left a comment

levithomason Oct 4, 2016 •

edited

Loading

jeffcarbs Oct 5, 2016

levithomason Oct 5, 2016

levithomason Oct 4, 2016 •

edited

Loading

jeffcarbs Oct 5, 2016

jeffcarbs commented Oct 5, 2016

jeffcarbs commented Oct 5, 2016

levithomason commented Oct 5, 2016 •

edited

Loading

jeffcarbs commented Oct 5, 2016

levithomason commented Oct 5, 2016

feat(shorthand): Custom shorthand proptypes #618

feat(shorthand): Custom shorthand proptypes #618

Conversation

jeffcarbs commented Oct 4, 2016 • edited Loading

codecov-io commented Oct 4, 2016 • edited Loading

Current coverage is 100% (diff: 100%)

levithomason commented Oct 4, 2016

Icon/Image

layershifter commented Oct 4, 2016 • edited Loading

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

levithomason Oct 4, 2016

Choose a reason for hiding this comment

jeffcarbs Oct 4, 2016

Choose a reason for hiding this comment

levithomason commented Oct 4, 2016

jeffcarbs commented Oct 4, 2016 • edited Loading

levithomason commented Oct 4, 2016

jeffcarbs commented Oct 4, 2016

levithomason commented Oct 4, 2016

levithomason left a comment

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

jeffcarbs Oct 5, 2016

Choose a reason for hiding this comment

levithomason Oct 5, 2016

Choose a reason for hiding this comment

levithomason Oct 4, 2016 • edited Loading

Choose a reason for hiding this comment

jeffcarbs Oct 5, 2016

Choose a reason for hiding this comment

jeffcarbs commented Oct 5, 2016

jeffcarbs commented Oct 5, 2016

levithomason commented Oct 5, 2016 • edited Loading

jeffcarbs commented Oct 5, 2016

levithomason commented Oct 5, 2016

jeffcarbs commented Oct 4, 2016 •

edited

Loading

codecov-io commented Oct 4, 2016 •

edited

Loading

layershifter commented Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

jeffcarbs commented Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason Oct 4, 2016 •

edited

Loading

levithomason commented Oct 5, 2016 •

edited

Loading