[core] Migrate nprogress to emotion #36181
Conversation
siriwatknp
added
core
| if (!theme.nprogress.color) { | ||
| throw new Error( | ||
| 'MUI: You need to provide a `theme.nprogress.color` property' + | ||
| ' for using the `NProgressBar` component.', | ||
| ); |
There was a problem hiding this comment.
I think there is no need to throw error if we use palette.primary.main from the theme.
Netlify deploy previewhttps://deploy-preview-36181--material-ui.netlify.app/ Bundle size report |
| @@ -1,10 +1,10 @@ | |||
| import * as React from 'react'; | |||
| import PropTypes from 'prop-types'; | |||
| import NProgress from 'nprogress'; | |||
| import { withStyles } from '@mui/styles'; | |||
| }), | ||
| '& .nprogress-bar': { | ||
| position: 'fixed', | ||
| backgroundColor: (theme.vars || theme).palette.primary.main, |
There was a problem hiding this comment.
The store uses a different value. https://github.com/mui/mui-store/blob/3933fb75c13040c53dc16f4ea4286f39e03b63ca/web/src/utils/theme.js#L125
How can developers configure it?
For the backstory, the component was created with the objective to expose it like https://mantine.dev/others/nprogress/. I think that it would be great to have it in the lab.
There was a problem hiding this comment.
The store uses a different value. https://github.com/mui/mui-store/blob/3933fb75c13040c53dc16f4ea4286f39e03b63ca/web/src/utils/theme.js#L125
It was theme.palette.primary.main before migrating to emotion. I think it should be the same.
How can developers configure it?
For the backstory, the component was created with the objective to expose it like https://mantine.dev/others/nprogress/. I think that it would be great to have it in the lab.
I think this is out of the scope of this PR but I do agree that it'd be useful to have it in the lab. I suggest to create a separate issue to keep track of it.
There was a problem hiding this comment.
@siriwatknp Actually, the store doesn't use NProgress.
I think this is out of the scope of this PR
Agree, but I think that we should get clarity on the path forward, so we can align the current solution with it. In this case, maybe the right approach would be:
- Migrate from
@mui/stylesto@mui/system(this PR) - Expose the component in the lab
- Remove the dependency on
nprogress - Use
stylesrather thanGlobalStylesso that developers can more easily use the sx prop to customize the component (unlocked by removing the dependency onnprogress).
There was a problem hiding this comment.
So, I think that we are good for the customization, we can delay it until later.
Another issue I can notice is the contrast: Is there enough between light <-> main to see movement? It feels like no
Before:
Screen.Recording.2023-02-15.at.12.04.45.mov
After:
Screen.Recording.2023-02-15.at.12.06.25.mov
There was a problem hiding this comment.
@oliviertassinari Fixed by using the same color as before.
There was a problem hiding this comment.
It looks great, happy to see us continue the migration to v5 and closer to make #22486 happen 👍
oliviertassinari
added
the
component: nprogress
| left: 0, | ||
| right: 0, | ||
| height: 2, | ||
| zIndex: (theme.vars || theme).zIndex.tooltip, |
There was a problem hiding this comment.
Could this be (theme.vars || theme).zIndex?.tooltip ? theme.vars.zIndex is undefined @siriwatknp
Also on the deploy preview of this PR: https://deploy-preview-36181--material-ui.netlify.app/joy-ui/getting-started/overview/
There was a problem hiding this comment.
Ahhh, you are right. On the other hand, I think we could add zIndex to Joy UI as well. Will open a fix now.
There was a problem hiding this comment.
The root cause seems to be that we have a theming isolation problem: Joy UI theme shouldn't be a parent of NProgress.
| borderRadius: '100%', | ||
| animation: `${muiNProgressPulse} 2s ease-out 0s infinite`, | ||
| }, | ||
| '& .nprogress-bar > div:first-child': { |
There was a problem hiding this comment.
This warns on all the pages of the docs & website:
Could we fix it?
There was a problem hiding this comment.
Oops! here is the fix => #36263
My goal is to remove
nprogressfrom the theme.Related to #22486