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.

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

[core] Prepare for material v6 #14143

Merged
merged 13 commits into from
Aug 21, 2024
Merged

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Aug 8, 2024

Part of #14055

Apply changes based on https://next.mui.com/material-ui/migration/migrating-to-v6/#add-styles-for-specific-color-modes.
Run the following command on packages and docs folders:

  • npx @mui/codemod@next v6.0.0/styled
  • npx @mui/codemod@next v6.0.0/sx-prop
  • npx @mui/codemod@next v6.0.0/theme-v6
    After running codemods, irrelevant changes have been removed and slight optimizations applied (i.e. replace border redeclaration with only overriding the borderColor)

I've also checked other usages of theme.palette.mode and tried adjusting them all (mostly docs) to play nicely with new API thus avoiding the possible dark mode flicker.

Actual changes on the code are limited, it's mostly docs.

@LukasTy LukasTy added the core Infrastructure work going on behind the scenes label Aug 8, 2024
@LukasTy LukasTy self-assigned this Aug 8, 2024
@mui-bot
Copy link

mui-bot commented Aug 8, 2024

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

Generated by 🚫 dangerJS against 0b2a17a

@cherniavskii
Copy link
Member

Great timing, I've just opened #14142 to run the tests against @mui/material@^6 😅

@LukasTy
Copy link
Member Author

LukasTy commented Aug 8, 2024

Yeah, I saw you opening that PR and decided to open the draft of moving to applyStyles. 😃

@LukasTy LukasTy marked this pull request as ready for review August 13, 2024 10:23
@LukasTy LukasTy requested review from a team August 13, 2024 10:23
opacity: 0,
},
]}
/>
Copy link
Member

Choose a reason for hiding this comment

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

Is this change required? It does not seem to make any difference, except that it now creates 2 objects instead of one 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

If I remember correctly, @siriwatknp confirmed that is is required for Pigment CSS to work, the previous syntax would not be supported by static extraction. 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Tagging @brijeshb42 to confirm.
If that's the case, we should document this in the migration guide.

Copy link
Member

Choose a reason for hiding this comment

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

This is required. Please have a look at my explanation

Copy link
Member

Choose a reason for hiding this comment

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

By the way, in cases such as this one in which there's one condition, you can remove the array

        sx={
          hasError
            ? {
                opacity: 1,
              }
            : {
                opacity: 0,
              },
        }

@@ -129,15 +129,15 @@ function ControlledColorRadio(props: any) {
<Check
fontSize="medium"
color="inherit"
sx={{
sx={(theme) => ({
Copy link
Member

Choose a reason for hiding this comment

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

Is this for future Pigment CSS support?
We seem to miss this in the migration guide https://next.mui.com/material-ui/migration/migrating-to-pigment-css/

Copy link
Member

Choose a reason for hiding this comment

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

AFAIK it's not included in the migration guide as it's covered by the codemod.

@siriwatknp, what do you think about adding an example for all cases covered by the codemod? Would there be too many?

Copy link
Member

Choose a reason for hiding this comment

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

For small cases like this, I can update the migration page but it should not block the v6.

Copy link
Member

@DiegoAndai DiegoAndai Aug 20, 2024

Choose a reason for hiding this comment

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

Let's create an issue to add the additional examples, but I agree not to block the stable release on this one. May I ask you to create it when you have some time?

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we can assume that everyone would be running codemod.
Also, having this covered in the guide is essential – we ran codemod, but I wasn't sure why this change even matters and I couldn't find any information about it.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I can update the codemod to link to the migration as well if that helps.

…erForm.tsx

Co-authored-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Lukas Tyla <llukas.tyla@gmail.com>
@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 15, 2024

@@ -29,26 +26,29 @@ const Root = styled('div')(({ theme }) => ({
border: `1px solid`,
borderColor: (theme.vars || theme).palette.error.light,
},
...theme.applyStyles('dark', {
Copy link
Member

Choose a reason for hiding this comment

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

I was curious about whether we could use applyStyles() or not. It was added with mui/material-ui#40667, released with @mui/material@5.15.7, we have this dependency:

"@mui/material": "^5.15.14",

since #12516, v7.0.0, so it looks likes yes. Cool

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I was pretty sure we can't use it 😅
But it turns out the minimum supported version guarantees that applyStyles will always be available 👍🏻

@LukasTy
Copy link
Member Author

LukasTy commented Aug 16, 2024

Will we need an isMuiV5() helper like for v5? https://github.com/mui/mui-x/pull/2108/files#diff-b4487b0ccf1b0178520560e5e9b18efbd0d4030106ae794bda6638be8cbdd526L36

From the initial investigation—it doesn't look like we'll need it @oliviertassinari, because all the changes are supported by the @mui/material version we have listed in peerDependencies. 🤔

Copy link
Member

@alexfauquette alexfauquette left a comment

Choose a reason for hiding this comment

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

CHnages on docs and charts look good to me

Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

Please change https://github.com/mui/mui-x/pull/14143/files#diff-d2872fe72d025095f6347b61c0ac177d659c43b2b669a58e9c935760fa1b602e to:

<Box
   sx={[
        rootProps.rowGroupingColumnMode === 'multiple'
          ? {
              ml: 0,
            }
          : theme => ({
              ml: `calc(var(--DataGrid-cellOffsetMultiplier) * var(--_depth) * ${theme.spacing(1)})`,
            }),
      ]}
  style={{
    '--depth': rowNode.depth
  } as any}
>

Copy link
Member

Choose a reason for hiding this comment

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

There are some components that I don't know how to migrate, e.g.

export const BarElementPath = styled(animated.rect, {
name: 'MuiBarElement',
slot: 'Root',
overridesResolver: (_, styles) => styles.root,
})<{ ownerState: BarElementOwnerState }>(({ ownerState }) => ({
stroke: 'none',
fill: ownerState.isHighlighted
? d3Color(ownerState.color)!.brighter(0.5).formatHex()
: ownerState.color,
transition: 'opacity 0.2s ease-in, fill 0.2s ease-in',
opacity: (ownerState.isFaded && 0.3) || 1,
}));

Any ideas?
@LukasTy @siriwatknp @DiegoAndai

Copy link
Member

@DiegoAndai DiegoAndai Aug 20, 2024

Choose a reason for hiding this comment

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

I think this should work:

export const BarElementPath = styled(animated.rect, {
  name: 'MuiBarElement',
  slot: 'Root',
  overridesResolver: (_, styles) => styles.root,
})<{ ownerState: BarElementOwnerState }>(() => ({
  stroke: 'none',
  transition: 'opacity 0.2s ease-in, fill 0.2s ease-in',
  opacity: 1, // might not be required, but I'm not familiar with the animation
  variants: [
    {
      props: { isHighlighted: true },
      style: ({ ownerState }) => ({
        fill: d3Color(ownerState.color)!.brighter(0.5).formatHex()
      })
    },
    {
      props: { isHighlighted: false },
      style: ({ ownerState }) => ({
        fill: ownerState.color
      })
    },
    {
      props: { isFaded: true },
      style: { opacity: 0.3 },
    },
  ],
}));

The props and ownerState are also available in the style callback, here's a demo: https://codesandbox.io/p/sandbox/epic-ioana-v8hmlc?file=%2Fsrc%2FApp.tsx%3A12%2C32

@siriwatknp can confirm if this is the correct usage.

Copy link
Member

Choose a reason for hiding this comment

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

This won't work with Pigment, the ownerState usage is still dynamic. It should be:

export const BarElementPath = styled(animated.rect, {
  name: 'MuiBarElement',
  slot: 'Root',
  overridesResolver: (_, styles) => styles.root,
})<{ ownerState: BarElementOwnerState }>(() => ({
  stroke: 'none',
  transition: 'opacity 0.2s ease-in, fill 0.2s ease-in',
  opacity: 1, // might not be required, but I'm not familiar with the animation
  fill: 'var(--_fill)', // the `_` is a convention in CSS to indicates that the variable is private.
  variants: [
    {
      props: { isFaded: true },
      style: { opacity: 0.3 },
    },
  ],
}));

<BarElementPath style={{
  '--_fill': isHighlighted ? d3Color(ownerState.color)!.brighter(0.5).formatHex() : color,
}}>

Copy link
Member Author

@LukasTy LukasTy Aug 21, 2024

Choose a reason for hiding this comment

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

@cherniavskii Supporting PigmentCSS is not in the scope of this PR.
"Full support" for it is in the plans only for TreeView and Pickers, which have most of the work done.
I've only applied changes that were made by codemod, but for full support, we potentially need way more work and attention for it.
Personally, I would prefer omitting any manual changes for Pigment support to reduce the scope of the PR, need for review and the potential of errors. 🙈
WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

@cherniavskii I've removed the additional changes you introduced to avoid potential problems.
They would not be enough for PigmentCSS support anyways. 🙈
I think that support for it should be handled separately.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, the example I gave here was out of curiosity 👍🏻

@LukasTy LukasTy merged commit e570b4b into mui:master Aug 21, 2024
18 checks passed
@LukasTy LukasTy deleted the prepare-for-material-v6 branch August 21, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants