mui-x
mui-x copied to clipboard
mui
[DataGrid] A focused row should always be in `renderedRows`.
Problem
When virtualization is enabled, the focused cells will be unmounted when they get out of range. This causes a problem if you are reordering or navigating with the keyboard.
inspiration
I looked at how AG-grid implemented their virtualization. and what I observed is they keep the focused Row always in the table.
https://user-images.githubusercontent.com/34584266/210151091-0f34ec09-be23-4f49-a5b1-5b07e914189f.mp4
Solution
the row which holds the focused cell should always be rendered, even if it's out of range it should always be in renderedRows
Fixes
- [x] DataGrid reorder with virtualization, Fixes #6165
- [X] keyboard navigation for a cell that is out of viewport(Vertical virtualization), Fixes #5750
Todo
- [ ] keyboard navigation for a
columnthat is out of range - [x] keyboard navigation for a cell that is out of viewport( horizontal virtualization)
- [x] Close #6765
Netlify deploy preview
Netlify deploy preview: https://deploy-preview-7357--material-ui-x.netlify.app/
Updated pages
No updates.
These are the results for the performance tests:
| Test case | Unit | Min | Max | Median | Mean | σ |
|---|---|---|---|---|---|---|
| Filter 100k rows | ms | 313.9 | 501.9 | 405.8 | 406.46 | 82.293 |
| Sort 100k rows | ms | 402.8 | 853.7 | 710.3 | 694.92 | 158.831 |
| Select 100k rows | ms | 193.6 | 268.5 | 204.8 | 215.7 | 27.371 |
| Deselect 100k rows | ms | 106.8 | 282.2 | 162.3 | 172.68 | 63.744 |
Generated by :no_entry_sign: dangerJS against 22ce1e5fcced33890ec513875cdd93931e59cca3
This pull request has conflicts, please resolve those before we can evaluate the pull request.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
I think this might fix a bug I'm having where clicking a button within a row column sometimes makes the scrollbar jump when virtualization is enabled, though I'm not 100%.
@m4theushw Could you review this PR? It would be great to merge it soon
This pull request has conflicts, please resolve those before we can evaluate the pull request.
@yaredtsy if you're still interested in merging this, can you merge/rebase master? I'll review as soon as it's done so we can get this in.
Note, this may be of interest to you to have some context: https://github.com/mui/mui-x/pull/9037#discussion_r1228750611
This pull request has conflicts, please resolve those before we can evaluate the pull request.
@romgrk I am having trouble with yarn proptypes it's not generating as the pipeline expected. mine is not adding .required. on required fields. is there something I must do?.
I'm not sure what's going on, the .isRequired flag should not be changed by this PR. Maybe the build script is confused. I'd try to play with it to see if something in the changes causes the confusion. As a last resort, fixing the propTypes manually would be fine.
The feature is working when the focused cell is above the rendered region, but not when it's below. To test it I've put a cell in edit mode, scrolled up, and at some point the cell goes away. I can provide a video if you can't reproduce it.
yes when it is in edit mode it wont scroll b/c scrollToIndexes, wont be called, also if you hit Tab it will not scroll.
Also I'm not sure what behavior we expect/want/accept when a user does a keyboard-input while the focused cell is outside of range
i have tried to adpat what ag-grid is doing.
Alright, I think this behavior is acceptable.
Could you address the comment I left above? Could you also add tests to cover this feature? Once that is done, this should be ready.