[DataGrid] Fix double-click issue #1919
Conversation
oliviertassinari
force-pushed
the
fix-4-checkbox-state
branch
from
3bfe9ce
to
c9151bd
Compare
oliviertassinari
added
bug 🐛
oliviertassinari
force-pushed
the
fix-4-checkbox-state
branch
from
a8f41dd
to
a329cfb
Compare
| expect(getColumnValues(1)).to.deep.equal(['Puma']); | ||
| fireEvent.click(selectAll); | ||
| // TODO fix, should be only 2 | ||
| expect(Array.from(apiRef.current.getSelectedRows().keys())).to.deep.equal([0, 1, 2]); |
There was a problem hiding this comment.
fine to use apiRef to assert the state but your test coverage would be higher if you check the Mui-selected css class on the row
There was a problem hiding this comment.
True, we can replace it. I had a similar thought last week on another PR. Then realized it was needed because of pagination/virtualization, but here, we don't.
On another note, I didn't use .to.have.keys() as in the other tests because when the test fails, the error message is crap (try removing the change in the source and see how the test behaves on HEAD)
There was a problem hiding this comment.
but your test coverage would be higher if you check the Mui-selected css class on the row
@dtassone My bad, no we can't, I got trapped 😁. The selected rows 0 and 1 are not visible, they are filtered. I would have needed to assert the "X rows selected" message at the footer.
I'm a believer that the quality of the tests ultimately drives the quality of the code
I was mostly interested in making sure that each bug has its corresponding failing test case. For instance, if we fix 3 bugs, and only cover one with the tests. Something diverges from the standard.
| }); | ||
| expect(getColumnValues(1)).to.deep.equal(['Puma']); | ||
| fireEvent.click(selectAll); | ||
| // TODO fix, should be only 2 |
There was a problem hiding this comment.
Yup, this will get fixed by removing React.memo after which we get the updated visible rows in GridHeaderCheckbox.
Fix the double click issue: #1141 (comment). In the recording, you can see that I click twice, and the state stays the same. checked indeterminate is the same state as unchecked indeterminate. In core v5 we removed the visual distinction to match screen readers.
Note that the fix is mostly removing code we don't need* (*assuming it's better to not react to clicks on the checkbox when there are no rows).
I worked on this because #1879 didn't seem to go in a rigorous direction. I'm mostly interested in the test case and isolation. I'm a believer that the quality of the tests ultimately drives the quality of the code.