reactabular / reactabular

A framework for building the React table you need (MIT)
http://reactabular.js.org/
MIT License
914 stars 142 forks source link

More React-like composing API? #272

Open wmertens opened 7 years ago

wmertens commented 7 years ago

I wonder what Reactabular would look like if all the DOM-related configuration was done via components.

Formatters

the column and header formatters are almost the same signature as React function components. They could instead accept all the parameters as props, and composition would be done via the children prop. Let's call such a component a View.

const colorColumn  = {
  property: 'color',
  Views: [({value}) => <ColorBox color={value}/>],
}
// ...later, we want to add a popup when you click the cell
const colorPickerPopup = ({value, rowData, children}) => (
   <div onClick={() => openPicker(rowData)}>{children}</div>
)
// can also be done with column extensions
colorColumn.Views.unshift(ColorPickerPopup)

Search input

Allow configuring the search by providing components that adhere to value, onChange(), except that onChange can be called with newValue directly instead of through a synthetic event (it's not easy to generate those and not really necessary either).

A column could have a SearchInput prop that is used for that particular column. Then it is really simple to provide a ternary input for Booleans, a select input for enums etc, when using searchtabular.Field and searchtabular.Columns.

Inline editing

An inline editor column extension could key off editable: true and inject a View that switches the cell to edit mode on click, using a component given as an Input prop, calling an onRowChange(row, property, value) callback that is provided to the table.

Row rendering

instead of the onRow(row, extra) function that adds properties to some component, accept a Row component that gets the same information via props and has to return a <tr> (because of html) that somehow incorporates the given children.

This one is a bit debatable, because wrapping it for adding functionality means having to clone the <tr> since you can't have anything but <tr> under <tbody>. Not sure if that is a huge performance issue though.

Final thoughts

These API changes could be non-breaking, e.g. formatters could be run first and their result passed to Views as children.

Also, note that all the other functionality of Reactabular stays untouched due to it not being involved in these code paths; long live composition :)

bebraw commented 7 years ago

Formatters

the column and header formatters are almost the same signature as React function components. They could instead accept all the parameters as props, and composition would be done via the children prop. Let's call such a component a View.

The old API was more of a convenience choice as often you are only interested in value, but going with an object would allow you to plug React components in without any problems so this could be a worthwhile change.

Instead of [(<label>, { rowData: <label>, column: <column>, columnIndex: <number> })] we would do just { label: <string>, rowData: <data>, column: <column>, columnIndex: <number> }].

Search Input

A column could have a SearchInput prop that is used for that particular column. Then it is really simple to provide a ternary input for Booleans, a select input for enums etc, when using searchtabular.Field and searchtabular.Columns.

This might need more examples. I kept the core decoupled from concerns like search on purpose while having the minimum amount of props. Maybe it would be better to try to build on top of the core and explore ideas like column extensions here?

Inline Editing

An inline editor column extension could key off editable: true and inject a View that switches the cell to edit mode on click, using a component given as an Input prop, calling an onRowChange(row, property, value) callback that is provided to the table.

Yeah, this is a good place for a column extension. No need to burden the core.

Row rendering

instead of the onRow(row, extra) function that adds properties to some component, accept a Row component that gets the same information via props and has to return a (because of html) that somehow incorporates the given children.

This one is a bit debatable, because wrapping it for adding functionality means having to clone the since you can't have anything but under . Not sure if that is a huge performance issue though.

onRow is a sore point of the current design and I don't like it very much. It was a necessity as I needed some way to intercept per row. My main problem with it is that it doesn't feel declarative.

Maybe the proper solution would be to scratch onRow and follow the components prop idea instead? I imagine we could do <Table.Body components={... extend row here ...} />and drop the prop. This would be a minor change (backwards compatible).

Going through components would skip the wrapping problem since you would essentially replace rows entirely. I think this should allow composition too so you could compose a row through the same interface.

Final thoughts

These API changes could be non-breaking, e.g. formatters could be run first and their result passed to Views as children.

Overall the changes seem pretty minor. Pushing formatters a notch further and eliminating onRow in favor of component based composition would feel like the most immediate wins to me. Perhaps the rest could be solved by building on top of the core (i.e., column extensions).

wmertens commented 7 years ago

Formatters

Instead of [(<label>, { rowData: <label>, column: <column>, columnIndex: <number> })] we would do just [{ label: <string>, rowData: <data>, column: <column>, columnIndex: <number> }].

Right, except that I would rename label to value so display components could be reused between headers and rows (academic maybe).

What are your thoughts on the attribute name Views with a capital letter to indicate components? It would also allow keeping formatters as-is for backwards compatibility.

Search

Yes, for me search is already decoupled, and I was proposing how to pass props that can be used by the column extensions.

It would be nice to have a diagram of the data flow in Reactabular. Something like, raw rows and a columns definition goes in, and then there is a transformation of the columns definition depending on what extensions you are using, and a transformation and filtering of the row data, before going into the table components that present the converted rows using the components defined in the columns.

Row rendering

Yes, I'd love it if you can specify your own Row/Cell renderer, and in fact I think it would be nice if those were direct props on Provider, Body and Header.

Final thoughts

Overall the changes seem pretty minor. Pushing formatters a notch further and eliminating onRow in favor of component based composition would feel like the most immediate wins to me. Perhaps the rest could be solved by building on top of the core (i.e., column extensions).

That was my feeling too, it's little corners in the API that grate a little bit, for the rest things are really nice.

bebraw commented 7 years ago

Formatters

Right, except that I would rename label to value so display components could be reused between headers and rows (academic maybe).

Makes sense. It's a better name. That label came from header, but going with value is nicer for sure.

What are your thoughts on the attribute name Views with a capital letter to indicate components? It would also allow keeping formatters as-is for backwards compatibility.

Even though it's a little painful for the users (not that many fortunately 👍), I would revamp formatters to the new scheme. Smaller API surface.

It would mean react-edit would need a major version bump, but overall this feels like a good change to make.

Search

Yes, for me search is already decoupled, and I was proposing how to pass props that can be used by the column extensions.

I think I'm going to need a little code example to see the light with this one.

It would be nice to have a diagram of the data flow in Reactabular. Something like, raw rows and a columns definition goes in, and then there is a transformation of the columns definition depending on what extensions you are using, and a transformation and filtering of the row data, before going into the table components that present the converted rows using the components defined in the columns.

I could draw one if that helps. Some of the processing is arbitrary and it depends entirely on the use case, but the internals are well specified.

Row rendering

Yes, I'd love it if you can specify your own Row/Cell renderer, and in fact I think it would be nice if those were direct props on Provider, Body and Header.

Ok, I would go with the components idea here. If you could set <Table.Body components={{ row: ... customize here ..., cell: ... customize here ... }} /> that might be enough and allow us to ditch onRow once and for all. This definition would override Provider one should it exist. It's not a difficult addition to make.

wmertens commented 7 years ago

So how about the following core API ([] denoting optionals):

Other things I noticed:

bebraw commented 7 years ago

reactabular-table components: ...

Looks good to me.

column definition: as before, but with additional Views that generates the children of Cell

I'm not sure about this. formatters effectively does this so I would merge the idea there to keep the API slim.

Provider injects the columns into context, and Header uses it, but search.columns doesn't use it…

Correct. One option would be to drop the context entirely. Less complexity although then you have to do more wiring yourself. That said, you could still implement the context idea on top of a lower level API like this.

extensions exposes bind which overrides the default Function.bind. Would a better name be compose?

That could very well work.

Would it not make more sense to put resizeable etc on the column and not on column.header? The whole column is resizeable even though the UI lives in the Header…

This has to do with the original design (no sticky header/body yet). The point was that width changes made to a header would cause body to resize as well. The design could change for sure.

When it comes to sticky and width syncing, I tried a simpler alternative in which the table captured the widths through event bubbling, but for some reason this killed the performance although the syntax was a lot nicer.

wmertens commented 7 years ago

I just thought of an issue: When you need to externally influence the rendering, you can't create new components, since that would cause the existing elements to be unmounted. You have to use props.

So table components can be custom for each table, but they should remain the same components for the lifetime of the table.

Therefore, the Table should accept extra props that it passes on to custom components.

To prevent unnecessary rerenders, the props should be granular, e.g. only for the HeaderCell or RowCell of column; for dev comfort, the props should also be general if required.

So something like a tableProps={{sharedProp}} columnProps={[null, null, {header: {...}, row: {...}}, null]} perhaps?

By passing props instead of a prop-returning function, it also makes figuring out when to update cells easier, just rely on the props.

bebraw commented 7 years ago

Could those extra props be passed through the column definition? That's what the current design does more or less if I understood right.

wmertens commented 7 years ago

Hmmm right, I tried that (I have a wrapper that sort of implements this) and it works but it means messing with the columns definition in the render cycle, which is a little annoying and ugly…

bebraw commented 7 years ago

That could be a design smell. One option would be to try to push props to a definition of their own.

wmertens commented 7 years ago

Yes, with props separate from columns, only the affected cells will be re-rendered.

how about headerProps and rowProps being arrays like the columns, e.g. the second header column gets the second item from headerProps merged into its props?

The only annoyance there though is that when you filter out columns you also need to filter out the relevant props. That would not be needed if the columns have a propKey property which selects the subsection of headerProps/rowProps they get…

bebraw commented 7 years ago

how about headerProps and rowProps being arrays like the columns, e.g. the second header column gets the second item from headerProps merged into its props?

Given there's columns, we could mimic the design and do props while mimicking the same shape. That gives easy shared props between header/cells while keeping the design similar to the old.

If the idea of maintaining a context is dropped, both could be split further and you would end up with <Table.Header props={props.header} .../> and such.

The only annoyance there though is that when you filter out columns you also need to filter out the relevant props. That would not be needed if the columns have a propKey property which selects the subsection of headerProps/rowProps they get…

Yeah, unless you fork the data after filtering. That was the idea I was getting at in the previous paragraph. We can manipulate the data close to the old way while being more explicit about how we pass it to React. That feels like a good compromise here.

wmertens commented 7 years ago

On Mon, Feb 6, 2017 at 4:06 PM Juho Vepsäläinen notifications@github.com wrote:

how about headerProps and rowProps being arrays like the columns, e.g. the second header column gets the second item from headerProps merged into its props?

Given there's columns, we could mimic the design and do props while mimicking the same shape. That gives easy shared props between header/cells while keeping the design similar to the old.

Yes, but header cells and row cells don't need the same data, so you'd be updating all the rows if only the header needs changing. May not be a real issue in practice though. I'd call them columnProps then?

If the idea of maintaining a context is dropped, both could be split

further and you would end up with <Table.Header props={props.header} .../> and such.

Hmm, I kinda like the context… How much extra "boilerplate" would that mean for general Table use?

The only annoyance there though is that when you filter out columns you

also need to filter out the relevant props. That would not be needed if the columns have a propKey property which selects the subsection of headerProps/rowProps they get…

Yeah, unless you fork the data after filtering. That was the idea I was getting at in the previous paragraph. We can manipulate the data close to the old way while being more explicit about how we pass it to React. That feels like a good compromise here.

I don't understand, the hidden columns still need to be removed from the columns given to the table, no? So the same thing needs to be done to props? (columnProps)

bebraw commented 7 years ago

Yes, but header cells and row cells don't need the same data, so you'd be updating all the rows if only the header needs changing. May not be a real issue in practice though. I'd call them columnProps then?

That's a good point. In a more granular design scu should kick in. Granular checks are easier to make.

Hmm, I kinda like the context… How much extra "boilerplate" would that mean for general Table use?

Any header/body would need its definitions through props.

I don't understand, the hidden columns still need to be removed from the columns given to the table, no? So the same thing needs to be done to props? (columnProps)

That's a great point. It's a syncing problem. One way to solve that would be to extract the concept of order and treat props/columns as objects. The third concept would unify these so you would get ['id', 'name'] kind of an array that would map to those object keys. To hide columns or re-order it, you would manipulate this particular structure.

wmertens commented 7 years ago

I don't understand, the hidden columns still need to be removed from the columns given to the table, no? So the same thing needs to be done to props? (columnProps)

That's a great point. It's a syncing problem. One way to solve that would be to extract the concept of order and treat props/columns as objects. The third concept would unify these so you would get ['id', 'name'] kind of an array that would map to those object keys. To hide columns or re-order it, you would manipulate this particular structure.

Yes, that would be much nicer, plus you'd have a key for the column cells in a row, so that moving or hiding columns doesn't result in a bunch of re-renders. Sorting state could also be stored in that latter structure.

wmertens commented 7 years ago

Ok I will summarize the proposed changes here, updating this comment if needed. I'll assume no context, to see where we get. I'm going to assume API breakage, choosing nice names, and perhaps we can add backwards compatibility to that.

General ideas

columns

This is an array of definitions:

[{
  id, // unique
  getValue: obj => …, // optional, only needed if value is not rowData[id]
  header: {views: [{Cell, props}], cellWrap = 'th', wrapProps, …},
  body: {views: [{Cell, props}], cellWrap = 'td', wrapProps, …},
  …
}]

The Cells and props will generally be the result of wrapping from column extensions. The id is used to refer to the columns and to retrieve the value from rowData.

views is an array of Cell components with optional props, which is rendered with

cellProps => views.reduce((children, {Cell, props}) => <Cell {...cellProps} {...props}>{children}/>, cellProps.children)

For example, the search extension would insert [{Cell: Highlight}] at the beginning of body.views and [{Cell: HeaderSearch, props:{onChange}}] at the beginning of header.views:

const HighLightCell = ({value, search}) => (
  <span>/* series of spans for highlighting generated from value and search */</span>
)
const HeaderSearch = ({search, onChange, children}) => (
  <div>
    {children}
    <input value={search} onChange={onChange}/>
  </div>
)

search is passed through propsByColumn[relevant id].header.search for each searchable column header, so there needs to be a getPropsByColumn() created from the extensions for augmenting the propsByColumn before passing it to Table.

Components

<Reactabular.Table [Table='table'] ...restProps/>

Renders the Table element with ...restProps. <Table/> could render a straight div as well, for example.

<Reactabular.Body columns [propsByColumn] data [Row='tr'] [props] [Wrap='tbody'] ...restProps/>

results in

<Wrap {...restProps}>
   <!-- for each rowData in data -->
   <Row key={rowData.id} {...props}>
      <!-- for each col in column -->
      <CellWrap key={col.id} {...col.body.wrapProps}>
        <Cell colId={col.id} rowData={rowData} value={rowData[col.id]} {...col.body.props} {...propsByColumn[col.id].body}>
          {rowData[col.id]}
        </Cell>
      </CellWrap>
   </Row>
</Wrap>

<Reactabular.Header columns [propsByColumn] [Row='tr'] [props] [Wrap='thead'] ...restProps/>

results in

<Wrap {...restProps}>
   <Row {...props}>
      <!-- for each col in column; rowData=columns.map(c=>c.id) -->
      <CellWrap key={col.id} {...col.header.wrapProps}>
        <Cell colId={col.id} rowData={rowData} value={rowData[col.id]} {...col.header.props} {...propsByColumn[col.id].header}>
          {rowData[col.id]}
        </Cell>
      </CellWrap>
   </Row>
</Wrap>

<Reactabular.Table data columns [propsByColumn]/>

Shortcut for the most common case:

<Reactabular.Table>
  <Reactabular.Header columns={columns} propsByColumn={propsByColumn}/>
  <Reactabular.Body columns={columns} data={data} propsByColumn={propsByColumn}/>
</Reactabular.Table>

(so do this if data is defined)

Note that thanks to the granular prop passing, only the things that actually need to change will get different props.

Example

const columnDefs = [
  {id: 'birthDate', sortable: true, initialSort: -1, body: {views: [{Cell: DateValue}]}},
  {id: 'name', sortable: true, searchable: true},
]
class BirthdaysTable extends React.Component {
  state = {}
  componentWillMount() {
    // just adlibbing here
    const getState = name => () => this.state[name]
    const onState = name => state => this.setState({[name]: state})
    this.exts = makeExtensions(
      sortable({getState: getState('sort'), onState: onState('sort')}),
      searchable({getState: getState('search'), onState: onState('search')}),
    )
    this.columns = this.exts.wrap(columnDefs)
    this.resolveRows = makeResolver(this.columns)
    this.rows = this.resolveRows(this.columns)
    // Also would need componentWillReceiveProps to update rows
  }
  render() {
    const {rows, columns} = this
    const data = this.exts.getRows(rows)
    const propsByColumn = this.exts.getPropsByColumn()
    return <Reactabular.Table {...{data, columns, propsByColumn}}/>
  }
}

Notes

bebraw commented 7 years ago

Hi,

That's a solid design. A couple of points:

Looking good overall. 👍

wmertens commented 7 years ago

I adjusted the text.

I'm a little unsatisfied with the extension state management; it seems a bit clunky now, but it needs to be flexible enough to store and update state anywhere (local, redux, url, server, …). How about expecting an object that has state and setState? Each extension would map to their own key, and it is probably easy to convert to whatever external state management desired. In the easy case, you'd then just pass this.

bebraw commented 7 years ago

getValue seems like a nice API, even for table-resolver. Perhaps there could be a sugar prop . "attributePath"? In any case, makeResolver can go in table-resolver. It would handle the wrapper class creation for rendering and/or WeakMap for reverse lookup

I have a feeling attributePath would be too weak.

If we expose sugar, might as well expose a function interface. I know it comes with some overhead but maybe not too much. I would rather go with getValue. I wonder if it would be possible to merge the concepts of id and getValue as value. id could be overloaded to accept a string and a function. Would that defeat the point of the design?

I'm a little unsatisfied with the extension state management; it seems a bit clunky now, but it needs to be flexible enough to store and update state anywhere (local, redux, url, server, …). How about expecting an object that has state and setState? Each extension would map to their own key, and it is probably easy to convert to whatever external state management desired. In the easy case, you'd then just pass this.

Going through a state and setState pair makes sense to me. I wrote similar APIs for lots of packages that are around reactabular. This gives a good amount of control and allows you to adapt to any system you like as long as you implement the adapter.

I suppose with good design you could share an adapter (redux, whatever) across different adapters (feels like a factory that returns the pair).

wmertens commented 7 years ago

So that initialization could instead look like

  componentWillMount() {
    this.extensions = makeExtensions(
      sortable({store: this}),
      searchable({store: this}),
    )
    this.columns = this.extensions.extendColumns(columnDefs)
    this.resolveRows = makeResolver(this.columns)
    this.rows = this.resolveRows(this.props.people)
    // Also would need componentWillReceiveProps to update rows
  }

(update: but see below)

Another thought: We can very easily extend this to also incorporate a footer, which could process the rows to calculate sums etc, just by providing the Footer element and the definitions in the columns. Summary extensions can do their thing when calculating the column props.

...Much thinking ensued. Here's more about how the extensions work:

Thinking about the information flow in a general case. We assume that our table display is a derivation from input data, column definition and extension list + state only. We assume that extensions are called on intermediate data in (reverse) list order for any processing steps.

I think I have everything, so this points to each extension defining adjustColumns, prepare, transform (any combo), based on given options if it is a factory. An almost always-needed extension would be resolve, not a factory.

To prepare the final table processor, take a list of extensions and compose with composeExtensions(list). You get {adjustColumns, prepare, transform} back.

Maybe column hide/move should also fully be an extension. The processor would keep the adjusted columns internally, so then the API would be

const store = this
const extensions = [pager({store}), sorter({store}), filterer({store})]
// calls extension.adjustColumns(columns)
const processor = makeProcessor(extensions, columns)
// calls extension.prepare(rows)
const prepared = processor.prepare(this.props.rows)
// calls extension.transform({rows, propsByColumn})
const {rows: data, columns, propsByColumn} = processor.transform(prepared)
return <Table {...{data, columns, propsByColumn}}/>

This assumes the store already being given to the initialized extensions. Perhaps the store can also be first class, so we get:

const store = this // or tableStoreFromRedux() or whatever
const extensions = [pager({perPage: 10}), sorter, filterer]
// calls extension.adjustColumns({columns, store})
const processor = makeProcessor({extensions, columns, store})
// calls extension.prepare({rows, store})
const prepared = processor.prepare(this.props.rows)
// calls extension.transform({rows, propsByColumn, store})
const {rows: data, columns, propsByColumn} = processor.transform(prepared)
return <Table {...{data, columns, propsByColumn}}/>

Examples:

So suppose you want to show a table with sections by month, each with their own footer. Can this be done using this model? 👉 Yes: You would section the row data to be per month and render a header/body/footer per month using shared columns/extensions/state.

Suppose you want tree data. How do you show? 👉 in the Transform step, hide hidden subtree rows, and in the Configuration step add a leftmost column that shows the position in the tree. To open a tree, store the state in the resolved row. For sorting/filtering, the tree extension has to be split in two, a hiding step before sort+filter and a subtree sort + parent restore afterwards.

Suppose you filter/sort/page on the server 👉 configure the sorter/filterer/pager extensions to only add the needed components, not do the transforms. Pass the relevant store state to the server, render result. No resolver extension needed.

...it looks like we can make Reactabular always easy, thanks to extensions…

bebraw commented 7 years ago

I think the only question I have at this point is that would it make to push the idea of resolving as an extension?

That would lead to design like this:

componentWillMount() {
  this.extensions = makeExtensions(
    sortable({store: this}),
    searchable({store: this}),
    resolvable(???),
  )
  this.columns = this.extensions.extendColumns(columnDefs)
  this.rows = this.props.people; // resolve later internally?
  // Also would need componentWillReceiveProps to update rows
}

Would this give enough access for a resolve pass, though?

wmertens commented 7 years ago

I agree, resolve should be an extension, I forgot to add it to the example

const store = this // or tableStoreFromRedux() or whatever
const extensions = [pageable({perPage: 10}), sortable(), filterable(), resolver]
// calls extension.adjustColumns({columns, store}) (e.g. resolver creates wrapper class)
const processor = makeProcessor({extensions, columns, store})
// calls extension.prepare({rows, store}) (e.g. resolver wraps in wrapper)
const prepared = processor.prepare(this.props.rows)
// calls extension.transform({rows, propsByColumn, store})
const {rows: data, columns, propsByColumn} = processor.transform(prepared)
return <Table {...{data, columns, propsByColumn}}/>
bebraw commented 7 years ago

Ok. How do you want to proceed with this? Can you start a PR? I don't mind reviewing and giving feedback but apart from that I'm a bit tight on time till I get the book out.

wmertens commented 7 years ago

I'll adjust the notes above and give it some more time to think about and then start a PR yes. Don't have much time either but I think it won't actually be that much work, mostly copying things around.

bebraw commented 7 years ago

Ok, cool. I have a feeling you did most of the hard work already. 👍