Add hover and select

[techniq]

@cvanem this looks great, thanks!

Let me take a deeper look this weekend and I'll try to get it merged and released. A few initial thoughts:

• It would be nice if our default cellSelected / cellHovered styles matched the material spec

  • https://material.io/archive/guidelines/components/data-tables.html#data-tables-interaction
  • https://material-ui.com/demos/tables/#sorting-amp-selecting

• I wonder if we should track selected within mui-table state, or if it should be handled externally with maybe a isCellSelected like this:

<MuiTable
  isCellSelected={(column,rowData) => this.state.selectedRowIds.some(id => rowData.id === id)}
  onCellClick={(column,rowData)=> {
    this.setState(prevState => {
      if (prevState.selectedRowIds.some(id => rowData.id === id)) {
        // remove
        return {
          selectedRowIds: selectedRowIds.filter(id => id !== rowData.id)
        }
      } else {
        // add
        return {
          selectedRowIds: [...selectedRowIds, rowData.id]
        }
      }
    })
/>

you could also track by data index position if you wanted. This state management is even easier when using immer. Another big benefit implementing cell selection this way would be to enable "select all" as you would just need to set this.setState({ selectedRowIds: data.map(d => d.id)} on the click of a checkbox, etc. You could also enable per-cell highlighting if you also checked the column argument passed to isCellSelected.

• I wonder if row hovering could be accomplished with just the index of the row within data. At that point we would only be added the isCellSelected and rowHover prop

I ended up spending more time looking at this than I initially planned :)

What are your thoughts on these changes?

[cvanem]

@techniq All valid points. My thoughts:

I agree that an external state is the way to go. The only drawback being slightly more work on user implementation, which is well worth it for all added benefits and customization potential you get.

I also agree with you that only the index should be used for row hovering. This allows hovering to work without a specified row id. It also works well considering the selected state is now externally managed.

Just a side note that doesn't really matter. I doubt we need to be too concerned with cell rendering performance, but I believe my implementation for determining if a cell is selected is going to be faster as it checks for selected status by directly accessing selected[id], as opposed to using find or some. Since we are moving it to an external state, the user can decide how they wish to implement it. It may be worth running a few performance tests with a large number of cells to see if either implementation has a significant advantage for purposes of writing example code. If a table has a large number of cells and each time a cell is rendered it has to iterate through a large list, it may get slow.

I have never used immer. I took a brief look at it but I need to get some hands on experience to understand how it would be used in this scenario.

Let me know what you need me to do as I don't want to duplicate any work. I think we are pretty much on the same page with your recommended changes.

[techniq]

@cvanem Sounds good. I have some other work I need to get done this weekend, but I'll see if I can get to this in the next few days. Thanks again for the PR. This will be a good addition (and something I've been meaning to get to, just haven't had the time yet.

[techniq]

@cvanem I just released 1.2.0 with the addition of isCellSelected and isCellHovered props to control a cell's styling based on these states. Take a look at the Hover and Selected stories and let me know what you think. This isn't quite as convenient but I think it's more powerful and supports more use cases without increasing mui-table's prop API too much.

[cvanem]

@techniq Looks good. I was able to plug it into my project with minimal effort. I did a little performance testing. With virtualization, I started to see performance delays around 250k rows. Without virtualization the delays start occurring around 200-300 rows. The delays are a result of a mix of selected/deselected rows while hovering or selecting rows. You can see the delays if you use a larger dataset on the Selected with checkbox example story (with no height set).

A few other items:

1. The story Selected "with checkbox" should probably be called "with checkbox and hover"

2. Also, the column hovering does not work if the user hover's over the column header. I am not sure if this was intentional?

This new functionality will be nice to have. Thanks for taking the time. I am going to play around with the external state handling code to see if it is possible to speed it up.

EDIT: It looks like the performance delays are occurring because the cellRender is getting fired for every cell in the table when a row is hovered. I am still investigating...

[techniq]

@cvanem thanks for the feedback. I addressed both of your items in version 1.2.1. Note, now rowData will no longer be null/undefined for header cells, but instead be an empty object. This now requires you to check the contents of rowData (ex. rowData.id && ...) instead of just the existence of rowData if you do not want row hovering for the header row. The stories were updated to reflect this so check the most recent commit for a better explanation :)

That's good to know about performance. I haven't done much performance tuning with mui-table yet, been waiting for bvaughn's React performance tools to get released and run into performance issues myself before I dug in too much. If you come across any improvements that can be made, please let me know.

You might find better performance using Immutable.js with it's structural sharing. I haven't used it much myself (usually use immer or plain javascript/destructing and shallow copy the array on write). You might be able to even mutate the state directly (array.push, array.splice) which is usually ill-advised in React, but might be worth it here (especially to prove if that's were the hotspot is).

Actually, as I was writing this, you might be better off using an object to take selectedRowIds instead of an array. Using { 1: true, 2: false, ... } and then flipping the boolean values. Would still need to shallow copy the object if adhering to immutable state guidelines, but you could check directly the value in isCellSelected without scanning the array for each cell using array.some, which I think is a good chance for a perf improvement.

[cvanem]

@techniq Thanks for the update. I investigated the performance issues with the non-virtual tables a little more. The performance of the external logic could probably be slightly improved, but it appears that the real cause of the delays is because the mouse events are setting the state of the parent component, which results in the cellRenderer being fired for every cell in the grid. The performance issues are not seen as easily with virtual tables because the # of cells rendered are only from the visible rows and not the entire dataset. Correct me if I am wrong, but I believe this is a limitation of how MultiGrid works. I don't see an easy way to trigger the cellRenderer for only the associated cell or associated row unless I am missing something.

That being said, the performance delays are really a non-issue for the majority of use cases. I am using virtual tables so I don't need to worry about it.

[techniq]

@cvanem I think the non-virtualization is just how React works. It always re-renders from the state-change down the component tree. You could implement shouldComponentUpdate to short-circuit the virtual dom diff as long as the check within shouldComponentUpdate is fast (typically can be with immutable state).

In general though, this is the main reason virtualized/windowed rendering is used. This is even more obvious in Material-UI 0.x which used inline styles and were less performant (lots of new object creation, etc). I used react-virtualized with my mui-downshift component to help alleviate the performance pains, as it was originally written for Material-UI 0.x.

Sounds like for now we're good enough, but maybe we'll revisit some performance optimizations in the future.