Add Table component

[lyzadanger]

This PR adds a Table component and examples, and makes a few adjustment to the underlying table pattern to support the component better.

Implementation is based on the LMS Table component.

Here is a video of it in action, including some keyboard navigation:

Table API changes:

• (Breaking): columns prop renamed tableHeaders; property className on TableHeader objects now named classes
• Additions (non-breaking): classes and tableClasses support

Table behavior and UI changes:

• Tables are wrapped in a Scrollbox component (in LMS' implementation, they had a wrapper div, but this has been abstracted and formalized here)
• When a row is selected that is partially or completely obscured by the (sticky) table header, the scrollbox is scrolled to assure the entire row is visible
• Column headers render while loading; items explicitly do not (in LMS implementation, everything displays, but also a loading spinner, which could lead to some funky combinations)
• This PR proposes using a slightly bluish grey (as defined in a proposed palette a few years ago) for table-row hover and selection colors
• When rendered inside of a parent with height constraints, the Table will not change height when loading or changing contents. A loading Spinner is absolute-centered, and the minimum Table height accounts for a Spinner.
• Row striping and hover are introduced, per suggestion

Fixes #157
Depends on #172

[codecov]

Codecov Report

Merging #174 (e1248ea) into main (b8ca20d) will not change coverage.
The diff coverage is 100.00%.

❗ Current head e1248ea differs from pull request most recent head 16a593c. Consider uploading reports for the commit 16a593c to get more accurate results

@@            Coverage Diff            @@
##              main      #174   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           13        14    +1     
  Lines          158       224   +66     
  Branches        55        75   +20     
=========================================
+ Hits           158       224   +66     

Impacted Files	Coverage Δ
src/components/Table.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b8ca20d...16a593c. Read the comment docs.

[esanzgar]

I found an anti-pattern in the Table component that should be reviewed. props should be considered read-only. In the Table component, selectedItem should not be modified. The Table component should have a state to track the item that is currently focused. It should be autonomous and re-render itself without the need of the parent. You can check this undesired behaviour by using selectedItem={null} in the TableComponents.js and interacting with the tables.

I also found that Table failed to focus on a hidden row if it was at the bottom of the table.

I left a number of other comments. They are merely suggestions, please, do not feel obligated to respond to these.

In the future, have you considered to create a simpler more basic table for scenarios where no selection of items are needed. This static table component should not have hovering effects or clickable behaviour, but still could benefit from the table styling.

[esanzgar]

Another way to move is to loop or cycle (when you reach top, you move to bottom if ArrowUp is pressed).

[esanzgar]

I also raised some concerns about touch devices. I think it would be worth considering (1) supporting iOS < 13 and (2) using a single touch to trigger onUseItem (when on mobile browsers).

[lyzadanger]

I found an anti-pattern in the Table component that should be reviewed. props should be considered read-only. In the Table component, selectedItem should not be modified. The Table component should have a state to track the item that is currently focused. It should be autonomous and re-render itself without the need of the parent. You can check this undesired behaviour by using selectedItem={null} in the TableComponents.js and interacting with the tables.

There was a verbal conversation today between @esanzgar , @robertknight and myself about the pattern implemented in this Table component (and likewise a few other components in the LMS project). The general pattern is that the parent component is responsible for all state: it sets props on the child component (here Table) indicating the current "active item", and one or more callbacks (onSelectItem, onUseItem in Table's case). To clarify, the Table component does not mutate any of its props, but instead informs the parent of interactions/changes via the callbacks. The parent component owns all state, and Table is essentially stateless.

We discussed a possible future need for a "dumber" Table, which wouldn't require state control. We're going to wait until a use case arises before implementing this, but we agreed that it may well be useful in the future.

I'll work on integrating @esanzgar 's other feedback!

[esanzgar]

Thank you @lyzadanger for explaining me that this is a controlled table and that the parent must own the state of the table and update the table accordingly. Some of my comments (especially about the tests) reflected my incorrect understanding.

In this regard, maybe the name ControlledTable could help others to disambiguate this potential confusion.

[lyzadanger]

@esanzgar Whew! That feedback put me through my paces. I still want to look again at a few details with a fresh brain tomorrow, but I'm going to go ahead and ping you for a re-review. Given the amount of changes I've integrated, I've pushed a fixup commit to help you with your re-review.

I haven't touched any of the keyboard/touch events stuff yet. Might humbly request that we push that out for a follow-up. I think some friction is arising from my approach here, which was to copy over the existing implementation of this component from LMS and do the minimum needed to generalize it. It's hard to find the right line of "good enough" for a common package, I admit!

[esanzgar]

Awesome! Good work.