Extract common 'listing table' behavior into an external module that exposes pure functions.

[cjcenizal]

In this PR I tried to extract and encapsulate the behavior (but not the UI) of this type of view to a) codify the behavior, b) de-duplicate code, and c) make this file shorter and easier to scan.

Completely unintentionally, I'm apparently borrowing concepts from Redux here (https://gist.github.com/gaearon/64e2c4adce2b4918c96c3db2b44d8f68#file-counter-js-L18, original context at https://medium.com/@dan_abramov/you-might-not-need-redux-be46360cf367#53d3).

Next, I would want to see if we could create abstractions for:

• onSort
• onDelete

I'd also like to take a closer look at how the table is being configured, and see if we can make it more explicit and transparent.

[cjcenizal]

I'm not entirely comfortable with the dependency on Pager that we have in this module, especially in this function. I wonder what it would look like if we only depended on pageOfItems instead.

[cjcenizal]

Some thoughts on designing Table components...

A Table is composed of two things: a TableHeader and a TableBody.

The TableHeader consists of a single row of cells. The TableBody consists of multiple rows, each of which consists of cells. So we can think about configuring both the TableHeader and TableBody with one-dimensional arrays (1 row of N cells).

Within the configuration arrays, each index will provide the configuration for a particular cell (TableHeaderCell or TableRowCell). We'll need to be able to configure specific properties of each cell:

• Cell content (children), an obvious one. This could literally be anything.
• Presentational properties, e.g. whether overflowing content should be clipped. These could change over time as we discover new UI requirements.

So the general idea is:

// A TableHeader instance containing TableHeaderCell instances.
const tableHeader = configureTableHeader();

// An array of TableRow instances, each of which contains TableRowCell instance.
const tableBody = configureTableBody(items);

This assumes a row-centric view of the table, which I think makes more sense than a column-centric view, since each row represents a single "model" or entity, and these are the primary units of abstraction in OOP or when modeling a domain.

If very similar instances of Tables exist in different parts of the UI, we could abstract out these configuration definitions into functions. We could call these functions with configuration parameters to customize specific parts of the table configurations for different contexts.

We could also abstract out the functions that render different parts of a Table, if that improves the code's readability.

[weltenwort]

I like the row-centric approach, because it appears to be more powerful (also applies to other representations like list-of-cards). Creating the whole table body in one go though feels very restrictive. It would make stuff like infinite loading and virtual scrolling a lot more inefficient/difficult to implement. How about providing "blueprints" for the header and rows to the table, which it "instantiates" using React.cloneElement. This way jsx provides a declarative way to configure the table. Different representations for different rows can then be delegated to row components that choose a particular way of rendering depending on the document:

(just a quick draft, expect typos)

class DashboardLandingPageTable {
  render() {
    const headerRow = (
      <ItemTableHeaderRow>
        <ItemTableHeader
          label="Name"
          onChangeSort={this.actions.sortBy("name")}
          // ...
        />
        <ItemTableHeader
          label="Created At"
          onChangeSort={this.actions.sortBy("creation_date")}
          // ...
        />
        // ...
      </ItemTableHeaderRow>
    );

    const bodyRow = (
      <ItemTableBodyRow>
        <ItemTableStringCell
          key="name"
        />
        <ItemTableDateCell
          key="creation_date"
          format="YYYY-mm-dd"
        />
      </ItemTableBodyRow>
    );

    return (
      <ItemTable
        bodyRow={bodyRow}
        headerRow={headerRow}
        items={items}
      />
    );
  }
}

function ItemTable({bodyRow, headerRow, items}) {
  return (
    <table class="kuiItemTable">
      <thead>
        { React.cloneElement(headerRow) }
      </thead>
      <tbody>
        { items.map((item) => React.cloneElement(bodyRow, { item })) }
      </tbody>
    </table>
  );
}

function ItemTableBodyRow({ children, item }) {

  if (item.confidential) {
    return (
      <tr class="">
        <td>You do not need to know</td>
      </tr>
    );
  } else {
    return (
      <tr class="tableRow--highlight">
        { React.Children.map(children, (child) => React.cloneElement(child, { item })) }
      </tr>
    );
  }
}

function ItemTableDateCell({ format, item, key }) {
  return (
    <td>
      { formatDate(item[key], format) }
    </td>
  );
}

[cjcenizal]

Cool! This is a really interesting idea. I love how readable this is. I wonder if there's a way for this to consume generic table components in the UI Framework, such as Table, TableHeader, SortableTableHeaderCell, TableRow, TableRowCell. These would simply enforce markup and class names.

[weltenwort]

Absolutely, I just made up the component names in a hurry. Although we should choose appropriate names to differentiate between representational components like <KuiTableRow>, that just enforce the css classes and something like the <ItemTableBodyRow>, that implement the { children, item } interface.

[stacey-gammon]

One of the reasons I went with a column centric approach was because it would be easier to implement moveable columns. If you wanted to give a header cell functionality to change it's position in the table, this would be difficult with a bodyRow react element like that. In the current column implementation, you just reorder the columns array, and it works.

If you made bodyRow and headerRow an array of cells, you could still rearrange pretty easily but you'd have to do some extra work to make sure they are in sync.

Rearranging columns doesn't really make sense in the landing page tables, but I think it would be useful functionality for the discover table.

What do you think if I modified @weltenwort's suggestion to still use the columns definition like the below?
I still don't love this because then ItemTable can't encapsulate the knowledge of moveable columns, which it could if it was passed just the columns.

class DashboardLandingPageTable {
  render() {
    this.columns = getDashboardColumns();
    const headerCells = columns.forEach(column => getHeaderCell());
    const headerRow = <ItemTableHeaderRow> { headerCells }</ItemTableHeaderRow>
    const bodyCells = columns.ForEach(column => getRowCell());
    const bodyRow = <ItemTableBodyRow> { bodyCells } </ItemTableBodyRow>

    return (
      <ItemTable
        bodyRow={bodyRow}
        headerRow={headerRow}
        items={items}
      />
    );
  }
}
... the same

@weltenwort I'm a little confused how this works with the item being passed in though. Would ItemTableStringCell be a react element that takes in an item props and embeds item.name inside? Or does it automatically somehow use the key to fill the inner children?

@cjcenizal I understand your thinking that the row is a single entity, but for a table, the row cells and the header cells are coupled - it doesn't really make sense for them to exist separately, IMO. Also think about giving the user the ability to add or hide columns. That also would be easy given the column centric approach.

[cjcenizal]

I think it would be ideal to separate the "column order" concern from the "table configuration" concern. Ideally, the second concern should provide an interface which can be driven by the first concern. Here's an example of how we can provide column ordering, while preserving a row-centric way of configuring the table.

EDIT: I re-read your suggestion and your suggestion also separates the two concerns. So I should say that my line of thought is, a) row-centric is a more malleable mental model for me, so I would prefer to try to stick with it if possible and, b) keeping the order of columns in the header and body in sync isn't too difficult (see example). What do you think of this?

// This array gets updated when the user drags and drops a header cell.
const cellOrder = [
  "id",
  "name",
  "date",
];

const headerCells = {
  "id": (
    <ItemTableHeaderCell
      key="id"
    />
  ),
  "date": (
    <ItemTableHeaderCell
      key="date"
    />
  ),
  "name": (
    <ItemTableHeaderCell
      key="name"
    />
  ),
};

const header = (
  <ItemTableHeaderRow>
    {cellOrder.map(cell => headerCells[cell])}
  </ItemTableHeaderRow>
);

const cells = {
  "id": (
    <ItemTableCell
      key="id"
    />
  ),
  "date": (
    <ItemTableCell
      key="date"
    />
  ),
  "name": (
    <ItemTableCell
      key="name"
    />
  ),
};

const bodyRow = (
  <ItemTableBodyRow>
    {cellOrder.map(cell => cells[cell])}
  </ItemTableBodyRow>
);

[stacey-gammon]

This isn't very readable imo, because I'd see this and think, why am I filtering here? What do you think of something like this:

this.setState(ListingTableActions.startFetching());
 this.props.fetch(newFilter).then((results) => {
      const items = results.hits;
		  const pagesCount = this.pager.getPagesCount(items.length);
      this.setState(ListingTableActions.setFilteredItems(this.state, items, pagesCount));
}.finally(() => {
  this.setState(ListingTableActions.endFetching());
}

This also gives us a place to show any errors.

What did you think of @weltenwort's suggestion of passing in setState? I ask because it could simplify this refactor if we passed in that function. Doesn't have to be tied to React, all the Actions class needs to know is to call the function with the new state.

Then we could further refactor to:

ListingTableActions.filter(this.setState, this.state, this.props.fetch, this.pager); 

or do you think that is too abstracted?

[cjcenizal]

I really like the names you're suggesting for the actions -- much more understandable!

I do think passing in setState is a step too far in terms of abstraction. This shifts the call site of setState from one place to another, and I think the only effect on someone reading the code is that now it's more difficult to figure out the role of filter. By keeping the call site of setState local, we form a relationship between it and filter. This relationship helps the reader intuit the role of filter (to derive and return a new state object that is somehow filtered).

I'm also hesitant to pass this.props.fetch into it. This would give it knowledge of network requests and asynchronous logic. A lot more complexity than it has now, but for what benefit?

[stacey-gammon]

Some rambling thoughts:

For the sake of the following explanation, note that I put back the namespacing and renamed the file listing_table_state => listing_table_actions.

How do you feel about having these state variable names both inside listing_table_actions and out here? I feel like they could get out of sync and why @weltenwort's selectors suggestion works well. Just now I accidentally had return { isFetching: true } inside ListingTableActions, but out here had isFetchingItems still.

I can see this being an easy mistake to make during a refactor. Say we are using ListingTableActions for dashboard too. Someone makes a refactor and updates the variable name in here and ListingTableActions but forgets that DashboadLandingPageTable uses it as well and forgets to update there.

I see a few options to help this:

1. Push getters into listing_table_actions, so this becomesListingTableActions.getIsFetchingItems(this.state);.
   Cons: A lot of extra text. Perhaps over-engineered? Could simplify this by having ListingTableActions hold a reference to this.state. Then can just do listingTableActions.getIsFetchingItems(). Well, now I am going around in circles because this is pretty much just what I had in the first iteration.

2. Create a typed state definition, ListingTableState so the variables are defined in a single location. A single source of truth if you will, in regard to the variable name. A single location for comments, a jsdoc definition, and reusable no matter what UI framework we are using.
   Could maybe use this like so:

constructor() {
  this.state = new ListingTableState();
}

Cons: not sure how react would handle a non-native object in state. Still going around in circles as this is also close to what I first had originally: this.state = { tableState: new ListingTableState() }. My first implementation violated the prefer primitives in state style decision we were talking about.

3. Implement @weltenwort's selectors.

... we can chat more about this in our meeting in a few minutes

[cjcenizal]

These ideas will be incorporated in a separate PR into the original branch.