Skip to content
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.

Sign up for GitHub

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

[DataGrid] Fix cell slot style override #11215

Merged
merged 9 commits into from
Dec 6, 2023
Merged

[DataGrid] Fix cell slot style override #11215

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
25e4138
[data grid] Fix cell slot style override
oliviertassinari Nov 27, 2023
e873b5c
romain's review
oliviertassinari Nov 27, 2023
54f87da
add unit test
cherniavskii Nov 28, 2023
9ca37c6
fix failing unit test
cherniavskii Nov 28, 2023
f342daf
update the test
cherniavskii Nov 29, 2023
ee5cefc
Merge branch 'master' into fix-style-override
cherniavskii Nov 29, 2023
3846e3f
Merge branch 'master' into fix-style-override
cherniavskii Dec 4, 2023
06d4b8f
simplify the test
cherniavskii Dec 4, 2023
f4f56e7
Merge branch 'master' into fix-style-override
cherniavskii Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions packages/grid/x-data-grid/src/components/cell/GridCell.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,7 @@ const GridCell = React.forwardRef<HTMLDivElement, GridCellProps>((props, ref) =>
isSelected,
rowId,
tabIndex,
style: styleProp,
value,
width,
className,
Expand Down Expand Up @@ -348,9 +349,10 @@ const GridCell = React.forwardRef<HTMLDivElement, GridCellProps>((props, ref) =>
maxWidth: width,
minHeight: height,
maxHeight: height === 'auto' ? 'none' : height, // max-height doesn't support "auto"
...styleProp,
};
return cellStyle;
}, [width, height, isNotVisible]);
}, [width, height, isNotVisible, styleProp]);

React.useEffect(() => {
if (!hasFocus || cellMode === GridCellModes.Edit) {
Expand Down Expand Up @@ -544,6 +546,7 @@ const GridCellV7 = React.forwardRef<HTMLDivElement, GridCellV7Props>((props, ref
onKeyUp,
onDragEnter,
onDragOver,
style: styleProp,
...other
} = props;

Expand Down Expand Up @@ -678,16 +681,18 @@ const GridCellV7 = React.forwardRef<HTMLDivElement, GridCellV7Props>((props, ref
opacity: 0,
width: 0,
border: 0,
...styleProp,
};
}
const cellStyle = {
minWidth: width,
maxWidth: width,
minHeight: height,
maxHeight: height === 'auto' ? 'none' : height, // max-height doesn't support "auto"
...styleProp,
};
return cellStyle;
}, [width, height, isNotVisible]);
}, [width, height, isNotVisible, styleProp]);

React.useEffect(() => {
if (!hasFocus || cellMode === GridCellModes.Edit) {
Expand Down
38 changes: 26 additions & 12 deletions packages/grid/x-data-grid/src/tests/components.DataGrid.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,18 +10,9 @@ describe('<DataGrid /> - Components', () => {

const baselineProps = {
rows: [
{
id: 0,
brand: 'Nike',
},
{
id: 1,
brand: 'Adidas',
},
{
id: 2,
brand: 'Puma',
},
{ id: 0, brand: 'Nike' },
{ id: 1, brand: 'Adidas' },
{ id: 2, brand: 'Puma' },
],
columns: [{ field: 'brand' }],
};
Expand Down Expand Up @@ -64,6 +55,29 @@ describe('<DataGrid /> - Components', () => {
expect(getCell(0, 0)).to.have.attr('data-name', 'foobar');
});

it('should not override cell dimensions when passing `slotProps.cell.style` to the cell', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romgrk Could you cherry-pick this test to the sticky headers PR?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I don't understand what it's testing? Aren't the styles empty both times? Also we're changing a bit the approach in sticky, we use CSS variables to set dimensions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but I don't understand what it's testing? Aren't the styles empty both times?

It doesn't matter what we pass in slotProps.cell.style (this is why it's an empty object).
What matters is that the cell shouldn't lose its dimensions which are set as inline styles (see the screenshot in #11215 (comment))

we're changing a bit the approach in sticky, we use CSS variables to set dimensions.

The CSS variables are passed as inline styles, so this test still makes sense there. If slotProps.cell.style is not properly merged with the internal inline styles, they will be overwritten 🙂

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like that test :| I think we should move the destructured props from this:

<div
  style={style}
  {...other}
>
  {children}
</div>

to this and call it a day:

<div
  {...other}
  style={style}
>
  {children}
</div>

Probably because I just spent weeks going through all sorts of tests, but I feel like our test suite might need some cleanup. And I don't like the idea that we keep adding tests for bugs that are very unlikely to reoccur, such as this one, because we keep paying more and more time to run it. And if we test for the style prop, why wouldn't we also test all the other props that are equally likely to be overriden?

Lmk what you think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a different note, about the test, IIUC this might be incorrect. We want to test that we preserve the cell dimensions but we never have a moment when slotProps.cell.style is undefined, therefore initialCellWidth will always return the same value, regardless if the dimensions are correct.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to test that we preserve the cell dimensions but we never have a moment when slotProps.cell.style is undefined,

Hmm, the initial render doesn't have any cell styles passed, so the cell width is the proper one.
After the button click, the cell styles are added. Am I missing something?

Maybe setProps instead of button click would be more readable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I double checked and the test fails if the fix is not applied:

AssertionError: expected 48.609375 to equal 100

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point, let's keep it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've picked up this test & fix on the sticky PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

function Test() {
const [cellProps, setCellProps] = React.useState({});
return (
<div>
<button onClick={() => setCellProps({ style: {} })}>Apply cell styles</button>
<div style={{ width: 300, height: 500 }}>
<DataGrid {...baselineProps} slotProps={{ cell: cellProps }} />
</div>
</div>
);
}

render(<Test />);

const initialCellWidth = getCell(0, 0).getBoundingClientRect().width;

const button = screen.getByRole('button', { name: /Apply cell styles/i });
fireEvent.click(button);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could maybe simplify the test. One render that assert the inline style applied to the cell could be enough. But yeah, the way it's written here is more resilient to future code changes.

Should we maybe check that in addition to not break styles, we can add new ones?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!


expect(getCell(0, 0).getBoundingClientRect().width).to.equal(initialCellWidth);
});

it('should pass the props from componentsProps.row to the row', () => {
render(
<div style={{ width: 300, height: 500 }}>
Expand Down