feat(Table): add table row shorthand #567

jeffcarbs · 2016-09-29T02:31:28Z

Fixes #565

Add shorthand for Table and subcomponents:

TableCell shorthand
TableRow shorthand
Table shorthand
Add example to test table shorthand

I wound up implementing almost exactly like I had laid out in this comment: https://github.com/TechnologyAdvice/stardust/issues/565#issuecomment-250344807

I really like the way this came out. I think it allows you to be incredibly succinct in the simplest case but give you full control over the entire table (save for the thead, tbody, tfoot) elements. I think this is a good example of how powerful these shorthand factories are.

To give some more examples, assume this data:

const tableData = [
  { name: undefined, status: undefined, notes: undefined },
  { name: 'Jimmy', status: 'Requires Action', notes: undefined },
  { name: 'Jamie', status: undefined, notes: 'Hostile' },
  { name: 'Jill', status: undefined, notes: undefined },
]

The most basic table would be:

const headerRow = [
  'Name',
  'Status',
  'Notes',
]

const bodyRow = ({ name, status, notes }) => [name, status, notes]

<Table headerRow={headerRow} bodyRow={bodyRow} tableData={tableData} />

Now if one of those cells needs customizing, you can change that cell to any valid TableCell shorthand:

// Shorthand for TableRow
const headerRow = [
  'Name',
  { icon: 'attention', content: 'Status' },
  <MyCustomTableHeaderCell />,
]

// Function that returns shorthand for TableRow
const bodyRow = ({ name, status, notes }) => [
  name || 'No name specified',
  { icon: 'attention', content: status }
  <MyCustomElement someProp={notes} />
]

In these examples, the array of cells is just a shorthand for TableRow, so you can return TableRow props or a your own custom component:

const bodyRow = ({ name, status, notes }) => {
  someRowProp: true,
  items: [
    name || 'No name specified',
    { icon: 'attention', content: status }
    <MyCustomElement someProp={notes} />
  ]
]

// Or
const bodyRow = (data) => <MyCustomRow data={data} />

codecov-io · 2016-09-29T02:55:50Z

Current coverage is 99.62% (diff: 100%)

Merging #567 into master will increase coverage by <.01%

@@             master       #567   diff @@
==========================================
  Files           118        118          
  Lines          1862       1874    +12   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           1855       1867    +12   
  Misses            7          7          
  Partials          0          0

Powered by Codecov. Last update bceb7ef...022dc8f

levithomason · 2016-09-30T04:14:32Z

This looks really great, thanks. I'll try to focus on a review tomorrow.

jeffcarbs · 2016-09-30T21:10:10Z

Added specs for the shorthand rendering that should bump the coverage back to 100%.

levithomason

Awesome, thanks so much for starting this. Let's iterate, it's going in a great direction. Shorthand is really paying off!

levithomason · 2016-09-30T16:45:01Z

src/collections/Table/TableRow.js

+  itemAs: customPropTypes.as,
+
+  /** Shorthand array of props for TableCell. Mutually exclusive with children. */
+  items: customPropTypes.every([


How about going with cells instead of items?

levithomason · 2016-09-30T18:39:25Z

docs/app/Examples/collections/Table/States/TableWarningShorthand.js

+import { Table } from 'stardust'
+
+const headerRow = [
+  'Name',


I think each row renderer should get access to the row element. How about going with the same pattern for the header/footer as the table body renderer?

const headerRow = (firstRow) => ({ items: [ 'Name', 'Status', 'Notes', ], })

The reason the body row renderer needs to be a function is that each row in the body is a function of an element in the tableData array. The header and footer rows are static as far as the Table is concerned so I'm not sure if it would make sense for them to be functions.

I think each row renderer should get access to the row element.

Also to clarify, headerRow is the row element, same with footerRow. bodyRow is a more of a "template" of what a row element looks like.

Consistency

I see that logic and it is valid thinking. What I'm mostly after here is consistency, where all "row" props are the same type / signature and behave the same. I think this is important for users. Especially given the props have the same naming convention, it might not be clear that they are doing different things.

I'm also assuming that we'll eventually have add column shorthand with cell renderers, which would again be functions with similar signatures. Having all table shorthand always be a function that receives some data and returns something I think is good.

Power

What I'm less after, is just the fact that functions are more powerful and flexible than configuration due their reusable and composable nature. Contrived example, I can imagine dynamic reusable header functions that do things like "alphabetically sorted", etc. Without a function, the user would have to manually sort the table data outside of the table and generate sorted headers every time. Whereas with a header row function, you can make a single reusable alphabeticallySortedHeader function.

I often find myself refactoring to add a function where configuration was provided so it can have more control / ability. Though, I can't say I've ever had to refactor a function down to configuration for any reason.

All said, I want to keep flexible. LMK if you still don't agree. I'd settle for a simple prop name update to help differentiate the renderer function from the static props:

<Table headerRow={} renderBodyRow={} footerRow={} />

👍

+1 for the rename of bodyRow => renderBodyRow to differentiate it more. Not completely opposed to making the other two functions in the future, but for now I'd prefer keeping it as-is. Perhaps when we add column/cell shorthands it will give a little more clarity.

I'm on board 👍

levithomason · 2016-09-30T21:25:21Z

src/lib/factories.js

@@ -46,7 +46,7 @@ export function createShorthand(Component, mapValueToProps, val, defaultProps =
  } else if (_.isPlainObject(val)) {
    type = 'props'
    usersProps = val
-  } else if (_.isString(val) || _.isNumber(val)) {
+  } else if (_.isString(val) || _.isNumber(val) || _.isArray(val)) {


(...submitting a little half baked, ran out of time on this comment but want to get the convo started)

I'm not sure we should pass arrays on to be mapped to props. It means every component might do something different with the array (or not handle it at all). If the factory does handle arrays, IMHO it is most intuitive to expect it to always create an array of elements.

const button = Button.create('Submit') // `key` derived from the string/number literal or `childKey` prop const buttons = Button.create(['A', 123]) const buttons = Button.create([{ content: 'A' }, { content: 123 }]) const buttons = Button.create([<Other>A</Other>, <Other>123</Other>])

Then we can standardize shorthand propTypes since all components with shorthand would be capable of handling arrays.

If the factory passes it along, we'd need to make sure every mapValueToProps handles arrays. Though, every component then might do something different with the array which isn't clear to the user. "How does X handle arrays again?" Also, not all components / mapValueToProps() may be able support arrays. It could make standard propTypes tough.

Lastly, when passing an array, you are creating an array of X components. If there is a need to pass that array through the X's props to some Y's shorthand, then the first component is just a pass through. My gut says there should be a "one level deep" design decision. Meaning, we would forgo the middle man and pass the shorthand directly to the Y instead of through X to Y.

...totally open on this, just thinking out loud.

To give a little more context, I came up with these shorthands by building upwards and trying to collapse where possible. So:

// This <Table.Cell>Name</Table.Cell> // became <Table.Cell content='Name' /> // This <Table.Row> <Table.Cell content='Name' /> <Table.Cell content='Status' /> <Table.Cell content='Company' /> </Table.Row> // became <Table.Row cells=['Name', 'Status', 'Company'] /> // This <Table> <Table.Header> <Table.Row cells=['Name', 'Status', 'Company'] /> </Table.Header> </Table> // became <Table headerRow={{ cells: ['Name', 'Status', 'Company'] }} />

If you think about our shorthand, generally it lets you pass a literal in place of a one-key object, e.g.

// One-key object <Label detail={{ content: 'Some detail' }} /> // Literal <Label detail={'Some detail'} />

So I figured if we can describe a Table.Row as a one-key object, we should be able to describe it as a "literal":

// One-key object <Table headerRow={{ cells: ['Name', 'Status', 'Company'] }} /> // Literal <Table headerRow={['Name', 'Status', 'Company']} />

So I'm not thinking about using arrays to let you build an array of some component (e.g. const buttons = Button.create(['A', 123])), but instead I'm thinking about it as a valid way of describing a component that has a list of things (e.g. TableRow has a list of cells that can completely describe that row).

Other examples:

// One-key object Menu.create({ items: ['menu item 1', 'menu item 2', 'menu item 3'] }) // Literal Menu.create(['menu item 1', 'menu item 2', 'menu item 3']) // One-key object List.create({ items: [<List.Item />, { content: 'item 2', description: 'item 2'}, 'list item 3'] }) // Literal List.create([<List.Item />, { content: 'item 2', description: 'item 2'}, 'list item 3'])

Thoughts?

I had a sudden moment of clarity thinking about this haha. There are really two separate types of shorthands we're talking about:

What is shorthand?

A way of describing component using a literal value, a props object, or an complete React.Element

Item Shorthand

What we've dealt with thus far. Describes an individual component that may or may not have children. If it does have children, the children tend to be heterogeneous/different (e.g. a label can have multiple children but they're all different: icon, image, content, detail, delete-button)

Valid propTypes:

string/number

Object (props)

Element

Example Components

Label

Icon

TableCell

Collection Shorthand

What this PR is introducing. Describes an individual component that has a set of homogenous children (e.g. a menu has 1+ items that are all the same).

Valid propTypes:

array (of "item shorthand"s for set of children)

Object (props)

Element

Examples

Menu

List

TableRow

I think almost everything is the same for both (e.g. they can use the same factory function). The differences are:

Which validations we apply for the "literal" case. Could just have customPropTypes.collectionShorthand and customPropTypes.itemShorthand (if we like these names).

How the mapValueToProps function maps a literal, but that's defined within the individual component itself so there shouldn't really be an issue there.

Cool, I think we're seeing the same split in types of shorthand. This is what I ran into here where I reference customPropTypes.shorthandSingle and customPropTypes.shorthandMultiple. I like collection a lot better than multiple though 😄.

So, I think we have 2 confirmed types of shorthand then. Both handle a props objects and elements identically. The difference is in what other values they accept and how they mapValueToProps:

Single item factory (Icon, etc.) also handles string/number mapping them to props.

Collection factory (Menu, etc.) also handles arrays mapping them to a single collection prop (items, cells, etc).

There is a 3rd group we can deal with later, since I think they need API redesign. Breadcrumb, Accordion, and possibly Feed, which take an array but do not map it to props. Instead, they create children in a special way, usually due to markup constraints. We'll save that for another day and time.

I'm working on the Feed updates and shorthand propTypes as I find time. Won't hold up this PR for that work. I can circle back and update when my work there is ready.

levithomason · 2016-09-30T21:28:27Z

src/collections/Table/TableRow.js

@@ -76,6 +93,20 @@ TableRow.propTypes = {
  /** A row may call attention to an error or a negative value. */
  error: PropTypes.bool,

+  /** An element type to render as (string or function). */
+  itemAs: customPropTypes.as,


Not sure of/if there is a solution yet, but I'd love to remove this prop somehow. Seems an indicator of a larger pattern problem. We had this on the Label as well, but were able to remove it with sub components.

One idea would be that the implementing component would pass the as prop / key in each item in the array so it is explicit. If that is too cumbersome, perhaps we need TableHeaderRow or something, similar to the two Table cells.

The Label had detail and detailAs which we merged into one:

<Label detail={{ content: 'x', as: 'div' }} />

This is a little more tricky since items is an array of shorthands so it wouldn't be straightforward to add an "as" prop to each one since they may all be literals. For reference, this it where it would have to be done:

{headerRow && <TableHeader>{TableRow.create(headerRow, { itemAs: 'th' })}</TableHeader>}

In this case, headerRow is a shorthand for a TableRow.

OK, let's save it for a later PR.

Here's an idea for the record. I have this same issue with FeedExtra. It has the need for two different shorthands, one that uses images and one that uses text. I'm thinking of just adding:

FeedExtra.createImages() FeedExtra.createText()

What if we added this?

TableRow.create() TableRow.createHeader()

The latter would set as on each element in the array. I think this touches on how collection shorthands are different than an item shorthand. A string or number is not valid in a collection, TableRow.create('not valid') //=> <Table items='not valid' />. So it seems we don't need a factory that only provides a mapValueToProps but instead/also a way to update each item. Perhaps with a default props object/function somehow. Again, this comment is for later reference.

levithomason · 2016-09-30T21:32:03Z

docs/app/Examples/collections/Table/States/TableWarningShorthand.js

+
+const TableWarningShorthand = () => {
+  return (
+    <Table celled headerRow={headerRow} bodyRow={bodyRow} tableData={tableData} />


API suggestion:

<Table renderHeaderRow={...} renderBodyRow={...} renderFooterRow={...} />

Eventually, I'd like to also add column/cell shorthand. These would disallow use of the row shorthand, but I think it would give the user the ability to map through their data in what ever way made the most sense to them.

Per more recent convo, ignore ^

levithomason · 2016-10-01T07:37:10Z

docs/app/Examples/collections/Table/States/TableWarningShorthand.js

+import { Table } from 'stardust'
+
+const headerRow = [
+  'Name',


Consistency

I see that logic and it is valid thinking. What I'm mostly after here is consistency, where all "row" props are the same type / signature and behave the same. I think this is important for users. Especially given the props have the same naming convention, it might not be clear that they are doing different things.

I'm also assuming that we'll eventually have add column shorthand with cell renderers, which would again be functions with similar signatures. Having all table shorthand always be a function that receives some data and returns something I think is good.

Power

What I'm less after, is just the fact that functions are more powerful and flexible than configuration due their reusable and composable nature. Contrived example, I can imagine dynamic reusable header functions that do things like "alphabetically sorted", etc. Without a function, the user would have to manually sort the table data outside of the table and generate sorted headers every time. Whereas with a header row function, you can make a single reusable alphabeticallySortedHeader function.

I often find myself refactoring to add a function where configuration was provided so it can have more control / ability. Though, I can't say I've ever had to refactor a function down to configuration for any reason.

All said, I want to keep flexible. LMK if you still don't agree. I'd settle for a simple prop name update to help differentiate the renderer function from the static props:

<Table headerRow={} renderBodyRow={} footerRow={} />

👍

levithomason · 2016-10-01T07:37:44Z

docs/app/Examples/collections/Table/States/TableWarningShorthand.js

+
+const TableWarningShorthand = () => {
+  return (
+    <Table celled headerRow={headerRow} bodyRow={bodyRow} tableData={tableData} />


Per more recent convo, ignore ^

levithomason · 2016-10-01T07:42:37Z

src/collections/Table/TableRow.js

+  itemAs: customPropTypes.as,
+
+  /** Shorthand array of props for TableCell. Mutually exclusive with children. */
+  items: customPropTypes.every([


levithomason · 2016-10-01T08:05:22Z

src/collections/Table/TableRow.js

@@ -76,6 +93,20 @@ TableRow.propTypes = {
  /** A row may call attention to an error or a negative value. */
  error: PropTypes.bool,

+  /** An element type to render as (string or function). */
+  itemAs: customPropTypes.as,


OK, let's save it for a later PR.

Here's an idea for the record. I have this same issue with FeedExtra. It has the need for two different shorthands, one that uses images and one that uses text. I'm thinking of just adding:

FeedExtra.createImages() FeedExtra.createText()

What if we added this?

TableRow.create() TableRow.createHeader()

The latter would set as on each element in the array. I think this touches on how collection shorthands are different than an item shorthand. A string or number is not valid in a collection, TableRow.create('not valid') //=> <Table items='not valid' />. So it seems we don't need a factory that only provides a mapValueToProps but instead/also a way to update each item. Perhaps with a default props object/function somehow. Again, this comment is for later reference.

levithomason · 2016-10-01T08:06:35Z

src/collections/Table/TableRow.js

+  /** Shorthand array of props for TableCell. Mutually exclusive with children. */
+  items: customPropTypes.every([
+    customPropTypes.disallow(['children']),
+    // Array of shorthands for MenuItem


MenuItem => TableRow, or just pull this comment entirely.

levithomason · 2016-10-01T08:08:32Z

Ugh, still getting used to the new way GH groups/shows/hides review comments. LMK if that last review is not clear. I did it on the files tab where I could see my review comments next to the past comments.

levithomason · 2016-10-02T01:06:43Z

Just ran into the need to pass array values to mapValueToProps in the Feed, TechnologyAdvice/stardust@cb5cb06. Just confirming it seems this is certainly the way to go after all.

* TableCell shorthand * TableRow shorthand * Table shorthand * Add example to test table shorthand

- TableRow: items => cells - Table: bodyRow => renderBodyRow

jeffcarbs · 2016-10-02T03:32:42Z

Made the requested changes, rebased and pushed up the latest. There are some further enhancements that I think can be addressed on subsequent PRs, so I think this is ready for final review 👍

ralphievolt · 2016-12-05T01:51:37Z

Just a question.. So populating my table with data from my props, I should use the short hand method as shown here
const tableData = [ { name: undefined, status: undefined, notes: undefined }, { name: 'Jimmy', status: 'Requires Action', notes: undefined }, { name: 'Jamie', status: undefined, notes: 'Hostile' }, { name: 'Jill', status: undefined, notes: undefined }, ]

as opposed to { this.props.data.map(item) => <Table.Row> <Table.Cell> ......

Thanks.

levithomason · 2016-12-05T16:20:40Z

Both are valid, whatever works best for you and your app 👍

http://react.semantic-ui.com/collections/table#warning-shorthand

jeffcarbs force-pushed the feature/table-shorthand branch from a4eb81c to 6a94bf3 Compare September 30, 2016 21:09

levithomason reviewed Sep 30, 2016

View reviewed changes

levithomason requested changes Oct 1, 2016

View reviewed changes

levithomason added enhancement needs author feedback labels Oct 2, 2016

jeffcarbs added 4 commits October 1, 2016 23:24

feat(Table): Table shorthand (#565)

06d9e53

* TableCell shorthand * TableRow shorthand * Table shorthand * Add example to test table shorthand

Fix specs

4c697a9

Add table shorthand specs

cde6a80

feat(table): Rename Table props

022dc8f

- TableRow: items => cells - Table: bodyRow => renderBodyRow

jeffcarbs force-pushed the feature/table-shorthand branch from 6a94bf3 to 022dc8f Compare October 2, 2016 03:28

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

levithomason approved these changes Oct 3, 2016

View reviewed changes

levithomason changed the title ~~feat(Table): Table shorthand (#565)~~ feat(Table): add table row shorthand Oct 3, 2016

levithomason merged commit 359bf48 into master Oct 3, 2016

levithomason deleted the feature/table-shorthand branch October 3, 2016 05:10

jeffcarbs mentioned this pull request Oct 3, 2016

RFC: Open questions around "Collection Shorthand" #604

Closed

jeffcarbs removed the ready for review label Oct 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(Table): add table row shorthand #567

feat(Table): add table row shorthand #567

jeffcarbs commented Sep 29, 2016 •

edited by levithomason

Loading

codecov-io commented Sep 29, 2016 •

edited

Loading

levithomason commented Sep 30, 2016

jeffcarbs commented Sep 30, 2016

levithomason left a comment

levithomason Sep 30, 2016

levithomason Oct 1, 2016

levithomason Sep 30, 2016 •

edited

Loading

jeffcarbs Oct 1, 2016

levithomason Oct 1, 2016

jeffcarbs Oct 2, 2016

levithomason Oct 2, 2016

levithomason Sep 30, 2016

jeffcarbs Oct 1, 2016

jeffcarbs Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016

levithomason Sep 30, 2016

jeffcarbs Oct 1, 2016 •

edited

Loading

levithomason Oct 1, 2016

levithomason Sep 30, 2016

levithomason Oct 1, 2016

levithomason Oct 1, 2016

levithomason Oct 1, 2016

levithomason Oct 1, 2016

levithomason Oct 1, 2016

levithomason Oct 1, 2016

levithomason commented Oct 1, 2016

levithomason commented Oct 2, 2016 •

edited

Loading

jeffcarbs commented Oct 2, 2016

ralphievolt commented Dec 5, 2016

levithomason commented Dec 5, 2016

feat(Table): add table row shorthand #567

feat(Table): add table row shorthand #567

Conversation

jeffcarbs commented Sep 29, 2016 • edited by levithomason Loading

codecov-io commented Sep 29, 2016 • edited Loading

Current coverage is 99.62% (diff: 100%)

levithomason commented Sep 30, 2016

jeffcarbs commented Sep 30, 2016

levithomason left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason Sep 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Consistency

Power

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

What is shorthand?

Item Shorthand

Valid propTypes:

Example Components

Collection Shorthand

Valid propTypes:

Examples

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeffcarbs Oct 1, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Consistency

Power

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

levithomason commented Oct 1, 2016

levithomason commented Oct 2, 2016 • edited Loading

jeffcarbs commented Oct 2, 2016

ralphievolt commented Dec 5, 2016

levithomason commented Dec 5, 2016

jeffcarbs commented Sep 29, 2016 •

edited by levithomason

Loading

codecov-io commented Sep 29, 2016 •

edited

Loading

levithomason Sep 30, 2016 •

edited

Loading

jeffcarbs Oct 1, 2016 •

edited

Loading

jeffcarbs Oct 1, 2016 •

edited

Loading

levithomason commented Oct 2, 2016 •

edited

Loading