-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[DataGridPro] Fix column pinning layout #14966
[DataGridPro] Fix column pinning layout #14966
Conversation
Deploy preview: https://deploy-preview-14966--material-ui-x.netlify.app/ |
@@ -86,10 +86,10 @@ describe('<DataGridPro /> - Detail panel', () => { | |||
await microtasks(); | |||
|
|||
const virtualScrollerContent = $('.MuiDataGrid-virtualScrollerContent')!; | |||
expect(virtualScrollerContent).toHaveInlineStyle({ | |||
width: 'auto', | |||
expect(virtualScrollerContent).toHaveComputedStyle({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed height
to flex-basis
in https://github.com/mui/mui-x/pull/14966/files#diff-74adfff8c7dc341a80f5a489eb850b9d566b1be041dfdf0e47a9848609f5ac53R537, and I makes more sense to check the actual height rather than specific inline style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment not specific to this in particular, but I feel like we often test the implementation too closely, e.g. we set a specific style and test for that style. Quite a few of our tests would be better as visual regression tests, they would be more resilient to implementation changes, and would catch actual regressions (like this one could have been caught if we had the visual regression test).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that's why I added a visual regression test for this one: https://github.com/mui/mui-x/pull/14966/files#diff-48fe2988003e6e416aaaed7d91bfb330c9f5d2b405f13244b9993e87a74b15de
return ( | ||
<Element | ||
{...props} | ||
className={clsx(classes.root, gridClasses['container--bottom'])} | ||
style={{ transform: `translateY(${offset}px)` }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for that because filler now grows
This reverts commit 991912c.
Fixes #14965