[data grid] Performance: DOM changes #12013
Conversation
romgrk
added
the
component: data grid
|
Deploy preview: https://deploy-preview-12013--material-ui-x.netlify.app/ Updated pages: |
oliviertassinari
changed the title
|
There are a few argos changes, I feel like the remaining ones are acceptable. The one that's really apparent is that sibling cells of dynamic height cells have their content top-aligned rather than centered, which isn't necessarily wrong, more like different. |
There was a problem hiding this comment.
Linked #3587 to make it easier to follow the history.
It would have probably been great to add a visual regression test in #3446. It seems to me that we are good on this PR, but it would have helped us move with more confidence.
Same thoughts with #3955, it's not clear that we have tested. Maybe it's a detail.
packages/x-data-grid/src/components/containers/GridRootStyles.ts
Outdated
Show resolved
Hide resolved
docs/data/migration/migration-data-grid-v6/migration-data-grid-v6.md
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't get this change in Argos: https://app.argos-ci.com/mui/mui-x/builds/17744/76165287
I cannot reproduce this in the preview deployment: https://deploy-preview-12013--material-ui-x.netlify.app/x/react-data-grid/api-object/#inside-the-data-grid
Any ideas? Could this be related to OS font rendering?
There was a problem hiding this comment.
I figured it's a text underline rendering issue caused by the change in display value, I would think it's an upstream chromium issue.
There was a problem hiding this comment.
@romgrk I cannot reproduce it on MacOS (Chromium: 121.0.6167.139).
Can you reproduce it on Linux?
There was a problem hiding this comment.
No, the underline is cropped cleanly:
There was a problem hiding this comment.
Looks like the vertical centering is gone:
- https://app.argos-ci.com/mui/mui-x/builds/17744/76165330
- https://app.argos-ci.com/mui/mui-x/builds/17744/76165366
Can we keep it by default?
There was a problem hiding this comment.
No, I couldn't find a way to preserve that while satisfying the other constraints. CSS doesn't do vertical centering well unless it's in flex/grid mode. Vertical padding would mess up the space available, and the trick for text cells was to set the line-height to height (copied from ag-grid).
There was a problem hiding this comment.
the trick for text cells was to set the line-height to height
In the linked demos, the cells impacted are text cells.
Should we add display: 'flex' then?
Also, could you mention this change in the migration guide?
There was a problem hiding this comment.
Yes, that's the one case I couldn't make work, but as I mention above, I don't feel it's necessarily wrong. Trying to fix it would introduce too much magic imho, so I'd leave that change as it is.
I've added a note to the guide.
There was a problem hiding this comment.
And for the demo in particular, I also wouldn't add display: 'flex' because I'd rather demo the default behavior.
There was a problem hiding this comment.
Alright, thanks for the clarification.
Signed-off-by: Rom Grk <romgrk@users.noreply.github.com> Co-authored-by: Olivier Tassinari <olivier.tassinari@gmail.com> Co-authored-by: Andrew Cherniavskyi <andrew@mui.com>
Closes #10570
Changes:
.cell > .cellContentDOM node. Fewer DOM nodes means a faster rendering.Before: https://deploy-preview-12012--material-ui-x.netlify.app/x/react-data-grid/#pro-plan
After: https://deploy-preview-12013--material-ui-x.netlify.app/x/react-data-grid/#pro-plan
Further help with #11866