[DataGrid] Fix focus cell visibility #256
Conversation
There was a problem hiding this comment.
Should we add a test cases to avoid regressions in the future? Any idea on how we could best write them? For instance, with karma we could call .getBoundingClientRect() and make sure the horizontal offset is correct. It should be easy to write, fast to run and easy to debug if there is a regression (hopefully). I think that we could also argue that puppeteer would make it even easier to debug (in case of a regression), however, we would first need to:
- add puppeteer in the CI, otherwise the test has no value
- figure out why the tests are slow to run, I suspect moving from storybook to next.js to run them will help a lot but I have no data to back this (only an intuition that by using code spliting, we will save a lot of time both during runtime in the CI and locally.
I think that we should avoid visual tests for this type of problems, it would be slower to run and less accurate in the assertion.
| @@ -65,7 +65,7 @@ export const useVirtualColumns = ( | |||
| const lastCol = getColumnFromScroll(lastScrollLeftRef.current + windowWidth); | |||
|
|
|||
| const visibleColumns = apiRef.current.getVisibleColumns(); | |||
| const firstColIndex = visibleColumns.findIndex((col) => col.field === firstCol?.field); | |||
| const firstColIndex = visibleColumns.findIndex((col) => col.field === firstCol?.field) + 1; | |||
There was a problem hiding this comment.
Is the name firstColIndex still accurate?
There was a problem hiding this comment.
I think that we should avoid visual tests for this type of problems, it would be slower to run and less accurate in the assertion.
Well you will need to check the scroll and calculate that the scrollLeft value is less than the offset value of the focused cell by a small amount. Also you will need to press arrow left a few times until the focus move to a cell well outside the viewport. Then go back to the first column....
For me, it's the kind of test that will be very difficult to understand and visualize in karma.
There was a problem hiding this comment.
Well you will need to check the scroll and calculate that the scrollLeft
I think that we can only call getBoundingClientRect and make sure .left === 0, I don't see the need to test scroll or only other value. Yes, the test would need to use the keyboard to get into the right condition. This sounds perfect, reproducing the real-life use case.
Regarding the pain to visualize in Karma, I agree with you. It's why I said: "I think that we could also argue that puppeteer would make it even easier to debug". Puppeteer has more potential for this type of visual issue, but it's yet to be untapped by making it something we can rely on (it's not the case yet because it's no integrated in the CI).
There was a problem hiding this comment.
Well, document.activeElement.getBoundingClientRect().left === 0 would work only if we remove the body margin, a quick POC:
it displays -5, which is KO.
There was a problem hiding this comment.
Testing the component within the doc is probably not the best. As the doc can influence on the component behavior, let say you have a css override in the doc that break/fix the component.
oliviertassinari
changed the title
oliviertassinari
added
bug 🐛
|
@dtassone I have updated the pull request metadata to follow the standard, what do you think about it? The changes:
|
Yeah, agree, I have only open the issue to keep track of it, I doubt it's important for us NOW (maybe later). And it's definitely different to this one. My objective was to make the point that regression tests are likely valuable for this problem. |
Fix #254