[DataGrid] Fix cell slot style override

[oliviertassinari]

I noticed this in #207 (comment), the style override is broken.

You can also reproduce from this demo https://mui.com/x/react-data-grid/components/#cell,

diff --git a/docs/data/data-grid/components/CellWithPopover.tsx b/docs/data/data-grid/components/CellWithPopover.tsx
index aa193d809..9d021ddb0 100644
--- a/docs/data/data-grid/components/CellWithPopover.tsx
+++ b/docs/data/data-grid/components/CellWithPopover.tsx
@@ -33,6 +33,9 @@ export default function CellWithPopover() {
         {...data}
         slotProps={{
           cell: {
+            style: {
+              background: "red",
+            },
             onMouseEnter: handlePopoverOpen,
             onMouseLeave: handlePopoverClose,
           },

https://codesandbox.io/p/sandbox/icy-glade-xd8hxx?file=%2Fsrc%2FDemo.tsx%3A35%2C18

Before: https://codesandbox.io/p/sandbox/busy-mccarthy-83skjy?file=%2Fsrc%2FDemo.tsx%3A3%2C58
After: https://codesandbox.io/p/sandbox/mystifying-tharp-38s8cj?file=%2Fsrc%2FDemo.tsx%3A3%2C58

[mui-bot]

Deploy preview: https://deploy-preview-11215--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against f4f56e7

[romgrk]

I've picked this change on the sticky PR which contains conflicts with this, this can be closed.

[cherniavskii]

Thanks @oliviertassinari!
I've updated the PR to target the master branch to fix the issue for v6, added a unit test, and applied the fix to GridCellV7 👍🏻

[cherniavskii]

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

[romgrk]

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.

[cherniavskii]

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 🙂

[romgrk]

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.

[romgrk]

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.

[cherniavskii]

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.

[cherniavskii]

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

AssertionError: expected 48.609375 to equal 100

[romgrk]

Fair point, let's keep it.

[romgrk]

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

[cherniavskii]

Thanks!

[oliviertassinari]

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?

[cherniavskii]

Done!