[DataGrid] Fix last row #1071
[DataGrid] Fix last row #1071
Conversation
There was a problem hiding this comment.
Thanks for the reproduction. Please always describe the problem a pull request is solving. It's important for blaming changes in the future (it's not infrequent we need to understand 1 or 2 years old changes). It's also important for reviews, we can't review without knowing what problem we solve.
In this case, I think that we should have a test case, it looks important. I'm not sure a visual test would be best because the problem and solution seem Math related, so more a unit or end-to-end one.
|
As discussed this will be tested using the regression image snapshot tests |
|
I added a story that scrolls to the bottom, just need to setup an image test |
| const apiRef = useGridApiRef(); | ||
|
|
||
| React.useEffect(() => { | ||
| const timeout = setTimeout(() => { |
There was a problem hiding this comment.
The visual tests are waiting in a useEffect: https://github.com/mui-org/material-ui-x/pull/1081/files#diff-df56773d39f0fa9c8f39611f1993bf7df81d046c5d055a47365fccb494407de0R63. It's not waiting for this timeout to run. It won't screenshot it correctly at the right time. So it doesn't work (at least shouldn't in theory or be flaky). What's the setTimeout for?
There was a problem hiding this comment.
we have to wait until everything is rendered before we can move the scroll down. It seems to only work in a timeout.
I tried with useLayoutEffect...
There was a problem hiding this comment.
I'm approving even if we don't have an automated test for this. We still have a reproduction that the next person that will blame this line can find to reproduce. It's still a lot more inefficient than having an automated test that can run quickly (not a screenshot) but it's better than nothing.
I really don't think that virtualization should be tested with visual regressions. Visual regression should be about any problem that is CSS related. This isn't one. @dtassone If you have time, please add an automated test, otherwise fine, we can live without, and delay the cost to later.
fix small issue on the last row rendering
https://material-ui-x.netlify.app/storybook/?path=/story/x-grid-tests-rows--scroll-issue