Enable functionality to limit maximum number of rows to select

[endrjuskr]

Closes #751.

Interface change:

Before:

isRowSelectable: (dataIndex) => boolean;

Now:

isRowSelectable: (dataIndex, selectedRows) => boolean;

[coveralls]

Coverage increased (+0.07%) to 75.421% when pulling 80b2347 on endrjuskr:feature/issue-751 into a5ab0da on gregnb:master.

[endrjuskr]

Question: How we would like to handle a situation (quite common) when a maximum number of selected rows is lower than a number of rows and a user clicks on the header (select all)?

Current behavior: Selects all and unclicking any of row disables this row to be selected again.

[gabrielliwerant]

Hello @endrjuskr. Thank you for taking an interest in contributing to the project!

I've been thinking about this feature, and I think what I'd like to do instead, is extend the functionality of isRowSelectable to include the selectedRows object. This way, devs can use that information to insert any custom logic that they want to enable/disable selecting a particular row. As of this moment, isRowSelectable only tracks the dataIndex of the current row about to be selected. Although all the selected rows could be tracked separately by the dev, adding this argument would make it easier.

The advantage this has is that it allows customization based on any logic the dev wants, and it extends an already existing API rather than create a new one. I'm a little wary of creating new APIs if we can use existing ones to do the same job, as I want to be careful about creating an overall API that is so big and complex that it's difficult to reason about and lacks discoverability for those new to the project.

How does that sound?

[endrjuskr]

Sounds reasonable. To be honest, I was not fully convinced about the approach that I proposed, mostly due to selecting all rows from header.

[gabrielliwerant]

Thank you for taking this on. There are a couple things to add before we can merge it, including an additional change needed on line https://github.com/gregnb/mui-datatables/blob/master/src/MUIDataTable.js#L1033 to make sure we're passing in the argument when updating rows:

const selected = isRowSelectable ? isRowSelectable(displayData[i].dataIndex, prevState.selectedRows) : true;

[gabrielliwerant]

@endrjuskr Almost there! We're still missing the selectedRows argument in what is now this line: https://github.com/gregnb/mui-datatables/blob/master/src/MUIDataTable.js#L1034.

[gabrielliwerant]

Hey @endrjuskr, thanks for the update, however, if you look at the comment, I was actually referring to the test on line 274. :)

[endrjuskr]

I hope this time I fixed correct test :)

Also split one test to check exactly if isRowSelectable actually determines selectability of row.

[gabrielliwerant]

Actually, I think we can eliminate this test, or recombine it. (or change it as I'll discuss below). It looks like it has a couple of issues once separated out. Mainly, it just seems to be testing whether or not the isRowSelectable function we create here in the test has the proper return values, which basically means that the test is testing itself. It doesn't test that the table works because it doesn't simulate an interaction with the table select rows.

So, if we just want to show that the function is called, the test below (on line 571) does that, so we could combine it back with that one.

If instead we want to show the positive case, where we demonstrate that a click on a selected row that is allowed by isRowSelectable works, then I think we should model it after the one on line 274 and simulate the clicks.

I leave it to you as for how you'd like to resolve this one.

[gabrielliwerant]

Looks great! Thank you so much for sticking through it and contributing.