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

Bump @mui/monorepo digest to 61c3cc9 #17062

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Mar 21, 2025

This bump fixes the dark mode flicker for MUI X docs.
All MUI X pages now use CSS theme variables.

@siriwatknp siriwatknp added the dependencies Update of dependencies label Mar 21, 2025
Copy link

Thanks for adding a type label to the PR! 👍

@mui-bot
Copy link

mui-bot commented Mar 21, 2025

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

Generated by 🚫 dangerJS against d07dd42

@siriwatknp
Copy link
Member Author

🤔 Not sure why argos is failing. I don't think it's related to the monorepo?

@alexfauquette
Copy link
Member

🤔 Not sure why argos is failing. I don't think it's related to the monorepo?

It's just flaky CI 👍

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

Nice work, thanks for addressing the theme and RTL switching performance and issues. 🙌 💯

@alexfauquette
Copy link
Member

I added a fix for this demo. It's the only one I found with dark theme issue

https://deploy-preview-17062--material-ui-x.netlify.app/x/react-data-grid/features/

image

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.

Thanks for taking care of it

I noticed some theme.applyStyle('dark', { ... }) and theme.applyDarkStyle({ ... }) is their one to prefer compare to the other?

Copy link
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

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

@alexfauquette
Copy link
Member

alexfauquette commented Mar 21, 2025

Thanks for your laser vision scan ;)

The same list with checkboxes to know which are done

For the tree view, I don't know what the CSS var approach is to switch light/dark theme. Cause currently, the switch is done by

const style = {
    '--tree-view-color': theme.palette.mode !== 'dark' ? color : colorForDarkMode,
    '--tree-view-bg-color':
      theme.palette.mode !== 'dark' ? bgColor : bgColorForDarkMode,
};

For the pickers custom field, I did not understand why it's not working.

@LukasTy
Copy link
Member

LukasTy commented Mar 21, 2025

This is not the first time we have had dark theme visual regressions, which need manual checking. 🙈
WDYT, would it make sense to run screenshot tests for both light and dark themes? 🤔

@alexfauquette
Copy link
Member

This is not the first time we have had dark theme visual regressions, which need manual checking. 🙈

We would need to check if it's ok to double the price with @oliviertassinari

Those error only occurs in specific PR (like this one) when we impact how the light/dark mode is handled by the docs.

If it was possible to manually trigger a screenshot from master and the current branch to compare them in dark mode, I would do it. But doing that for all PRs looks overkill

@siriwatknp
Copy link
Member Author

@siriwatknp do you have time to fix these or should we split the fixes among the team? 🤔

I can fix it one by one, thank you so much for the list @alexfauquette @LukasTy

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 21, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@LukasTy
Copy link
Member

LukasTy commented Mar 21, 2025

The root cause seems to be the fact that theme.palette.mode resolves to light regardless of the selected mode. 🙈
Is it expected after the latest changes @siriwatknp?

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 21, 2025
@@ -77,15 +82,19 @@ const CustomTreeItem = React.forwardRef(function CustomTreeItem(props, ref) {
status,
} = useTreeItem({ id, itemId, children, label, disabled, rootRef: ref });

const style = {
'--tree-view-color': theme.palette.mode !== 'dark' ? color : colorForDarkMode,
Copy link
Member

Choose a reason for hiding this comment

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

This solution could still work if theme.palette.mode returned the actual mode set on docs... 🙈 🤷

Copy link
Member

Choose a reason for hiding this comment

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

The idea is to delegate the light/dark to CSS only. Otherwise, when you update the theme mode all the Demo ThemeProvider will re-render.

The new Demo theme provider that erase custom theme tokens:

https://github.com/mui/material-ui/pull/45386/files#diff-7527a515d20eff239825c53744ce9602043624db93e0a7275943c9fa95256bfbR32-R45

If some packages plan to have demo that do not support CSS-vars we could have a dedicated Demo wrapper that pass the mode token with something like

{{"demo": "NeedThemeToken.js", "notCSSVarsCompatible": true }}

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 23, 2025

If it was possible to manually trigger a screenshot from master and the current branch to compare them in dark mode, I would do it. But doing that for all PRs looks overkill

Yeah, let's not check all the images, this sounds really slow for the CI time to complete. Actually, thinking about it, we might as well consider how to be more efficient: mui/mui-public#290.

We could have one dark mode test, one that makes sure that the dark/light mode integration is not all broken.
To mind that this was not needed before, I think because when we change dark mode logic, we test it on the docs, and X is supposed to fully integrate with Material UI, so not diverge in any meaningful way that could create bugs.

@siriwatknp
Copy link
Member Author

siriwatknp commented Mar 24, 2025

@LukasTy @alexfauquette 2 things that are not working

I'm fixing both from the core side. I think ideally, there should be no change in this PR, if you are okay I would like to revert the changes after fixing the code side.

@alexfauquette
Copy link
Member

if you are okay I would like to revert the changes after fixing the code side.

I'm ok. I extracted the olny modification that is not docs only related #17106

But we will need to find a solution to those issues if we want to remove the forceThemeRerender fro X docs

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 24, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@siriwatknp
Copy link
Member Author

@LukasTy @alexfauquette Updated the monorepo with mui/material-ui#45661 and the fix on MUI System, both of these are fixed!

Let me refine a bit and add visual regression tests in mui/material-ui#45661, will let you know when this PR is ready for the final review.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Mar 24, 2025
@LukasTy
Copy link
Member

LukasTy commented Mar 25, 2025

@LukasTy LukasTy mentioned this pull request Mar 25, 2025
1 task
Comment on lines +3 to +10
- https://mui.com/system/styles/advanced/
- https://mui.com/system/styles/advanced/#class-names
- https://mui.com/system/styles/advanced/#server-side-rendering
- https://mui.com/system/styles/advanced/#theme-nesting
- https://mui.com/system/styles/api/#creategenerateclassname-options-class-name-generator
- https://mui.com/system/styles/api/#stylesprovider
- https://mui.com/system/styles/api/#themeprovider
- https://mui.com/system/styles/basics/
Copy link
Member

Choose a reason for hiding this comment

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

These probably need updating on the mui/material-ui repo side. 🤔

cc @mui/core

Copy link
Member

Choose a reason for hiding this comment

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

already on it mui/material-ui#45677

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Update of dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants