Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(Table): add shorthand for generating tables #565

Closed
levithomason opened this issue Sep 28, 2016 · 2 comments
Closed

feat(Table): add shorthand for generating tables #565

levithomason opened this issue Sep 28, 2016 · 2 comments

Comments

@levithomason
Copy link
Member

The Table has been updated to our v1 API in #451. This was done with the sub component API only. We now need to add shorthand for generating tables.

Our previous TableColumn implementation was inspired by Facebook's fixed-data-table. @jcarbo has also put forth many great starter ideas in this comment.

Though it is more work, I am leaning toward the suggested row based approach (last example from jcarbo) because it offers more control. Or at least providing this as one of the shorthand options. With the old TableColumn approach, here at TA we ran into issues by not being able to control the actual row or cell that was rendered.

I think providing "renderer" props that expect table elements to be returned is the way to go. We could even provide multiple "renderer" props that would allow varying levels of control. Even along side a columns prop that let you define columns like we used to. This way, you can have several approaches to accomplish the most common goals.

@jeffcarbs
Copy link
Member

Edit I wound up pseudo-coding way more than I intended (see the bottom section) haha. If you think this makes sense I can take a pass at implementing this. Might be easier to talk about once there's actual code written.


The tricky part here is going to be the body row since it's going to be based on data for a given row. The header/footer rows are just going to be static, which makes them slightly easier. What about this:

// Table usage.
<Table
  headerRow={rowShorthand}
  bodyRow={functionThatReturnsRowShorthand}
  footerRow={rowShorthand}
/>

Not setting headerRow or footerRow would leave out the thead or tfoot. tbody would always exists, just possibly be empty.

This would mean a Table.Row would need to know how to render itself. I'm thinking as items prop that is an array of shorthands for Table.Cell, either using the as within each item to specify Table.HeaderCell or possibly cellAs which would make it a little easier for the Table shorthand case. I'll try to explain...

Row shorthand
We would need to update the shorthand factory to treat an array as a literal, but I'm thinking:

TableRow.create = createShorthandFactory(TableRow, items => ({ items }))

So you could theoretically do this:

<Table
  headerRow={['name', 'status', 'notes']}
  bodyRow={(props) => [props.name, props.status, props.notes]}
  footerRow={[{ colspan: 3, content: 'Some footer content' }]}
/>

Same example, but adding props to the TableRow for each row of the body:

<Table
  headerRow={['name', 'status', 'notes']}
  bodyRow={(props) => ({ positive: status === 'paid', items: [props.name, props.status, props.notes] })}
  footerRow={[{ colspan: 3, content: 'Some footer content' }]}
/>

I'm using items to be consistent with our other array-of-shorthands, but could just as easily be cells which may make it a tad more clear.

Pseudo code for the implementation:

// Table
renderHeader () {
  return (
    <TableHead>
      TableRow.create(this.props.headerRow, { itemAs: 'th' })
    </TableHead>
  )
}

renderBody () {
  return (
    <TableBody>
      {this.props.tableData.map((item, index) => TableRow.create(this.props.bodyRow(item, index)))}
    </TableBody>
  )
}

renderFooter() {
  return (
    <TableFooter>
      TableRow.create(this.props.FooterRow)
    </TableFooter>
  )
}

render () {
  <Table>
    {headerRow && this.renderHeader()}
    {this.renderBody()}
    {footerRow && this.renderFooter()}
  </Table>
}

// TableRow
renderItems () {
  return this.props.items.map((item) => TableCell.create(item, {
    // Make 'itemAs' a tableRow prop to avoid needing to add the prop to each
    // list item individually. If the user passed an 'as' prop already within
    // 'item' it would still win.
    as: itemAs 
  }))
}

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

  <ElementType {...rest} className={classes}>{this.renderItems()}</ElementType>
}

jeffcarbs added a commit that referenced this issue Sep 29, 2016
* TableCell shorthand
* TableRow shorthand
* Table shorthand
* Add example to test table shorthand
jeffcarbs added a commit that referenced this issue Sep 29, 2016
* TableCell shorthand
* TableRow shorthand
* Table shorthand
* Add example to test table shorthand
@jeffcarbs
Copy link
Member

@levithomason - I wound up implementing this and I think it came out really well. Check out the PR referenced above ^

jeffcarbs added a commit that referenced this issue Sep 30, 2016
* TableCell shorthand
* TableRow shorthand
* Table shorthand
* Add example to test table shorthand
jeffcarbs added a commit that referenced this issue Oct 2, 2016
* TableCell shorthand
* TableRow shorthand
* Table shorthand
* Add example to test table shorthand
levithomason pushed a commit that referenced this issue Oct 3, 2016
* feat(Table): Table shorthand (#565)

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

* Fix specs

* Add table shorthand specs

* feat(table): Rename Table props

- TableRow: items => cells
- Table: bodyRow => renderBodyRow
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants