[DataGrid] Fix cell height after changing grid's density #1819
Conversation
DanailH
added
bug 🐛
| @@ -105,6 +115,11 @@ describe('<DataGrid /> - Toolbar', () => { | |||
| expect(screen.getAllByRole('row')[1]).toHaveInlineStyle({ | |||
| maxHeight: `${Math.floor(rowHeight * COMPACT_DENSITY_FACTOR)}px`, | |||
| }); | |||
|
|
|||
| // @ts-expect-error need to migrate helpers to TypeScript | |||
| expect(screen.getAllByRole('cell')[1]).toHaveInlineStyle({ | |||
There was a problem hiding this comment.
we could just load the maxHeight and do an equal assert to avoid the ts-error
There was a problem hiding this comment.
Yes but that tests the internal, not the visual part. For me this way the test is more secure. If we change the way the height of the cell is set then if we check the maxHeight the test will stop working, no?
There was a problem hiding this comment.
It looks like the bug resonates with #1618 (comment).
Regarding the solution, did we consider to 1. only have a height property on the row, not on the cells? 2. No min-height/max-height, only height?
That was the easiest way to solve the issue. Regarding 2. it will still be the same problem I think as even if we only have height it still needs to come from the state and be based on the row height. Option 1. is more favorable. I will see if the effort of depending only on the row height is too big. |
|
@DanailH Yeah, 1. and 2. are a bit off-topic. I was raising them as they seemed possible opportunities to improve the logic. |
|
I have updated #1618 (comment) to keep track of the bugs created by the |
Fixes #1776
The problem was related to grabbing the height from the state inside
GridRowCellscomponent which is memoized. Passing it as a prop solves the issue. I extended the existing density tests to cover that case.Preview: https://deploy-preview-1819--material-ui-x.netlify.app/components/data-grid/accessibility/#density-selector