-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[material-next][Snackbar] apply MD3 designs #39631
[material-next][Snackbar] apply MD3 designs #39631
Conversation
Netlify deploy previewhttps://deploy-preview-39631--material-ui.netlify.app/ @mui/material-next: parsed: +1.38% , gzip: +1.24% Bundle size reportDetails of bundle changes (Toolpad) |
2a9538c
to
3dea297
Compare
I'd like @DiegoAndai to review this. I am not aware of the Snackbar MD3 plan. |
@Best-Sardar is this ready for review? |
Hi @DiegoAndai, |
3dea297
to
c4f19f3
Compare
Hi @DiegoAndai, |
That's good 👍🏼 let's keep this PR only for TypeScript and have a separate one for MD3 design.
I didn't understand the question. The Snackbar's button is the text Button variant, right? |
In Figma designs, the action button has an inverse-primary text color. In the API designed for the snackbar, the actions are received as a ReactNode through props. Since these buttons are passed as props to the SnackbarContent from the outside, I can't modify their colors (without forcing that). Actually, the actions button are not text button with primary color. |
I understand now, thanks! Let's leave that for the user. As they provide the Button, they can choose the color they use. So, we shouldn't add the color in the SnackbarContent. We could create an issue to add this Regarding this Typescript PR, should we review it already? |
0a1090b
to
65d4200
Compare
Hi @DiegoAndai, There are a few things I need to tell you:
The overall changes have been made and are ready for review. |
docs/data/material/components/snackbars/SnackbarMaterialYouPlayground.js
Outdated
Show resolved
Hide resolved
@@ -3,13 +3,13 @@ import * as React from 'react'; | |||
import PropTypes from 'prop-types'; | |||
import clsx from 'clsx'; | |||
import { unstable_composeClasses as composeClasses } from '@mui/base/composeClasses'; | |||
import { emphasize } from '@mui/system'; | |||
import styled from '@mui/material/styles/styled'; | |||
import useThemeProps from '@mui/material/styles/useThemeProps'; | |||
import Paper from '@mui/material/Paper'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component depends on Paper
, I wonder if this will be a blocker for implementing the MD3 design 👀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Best-Sardar Thanks for working on this ~ I have left some comments
@@ -1,8 +1,11 @@ | |||
import * as React from 'react'; | |||
import { SxProps } from '@mui/system'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { SxProps } from '@mui/system'; | |
import { SxProps } from '../styles/Theme.types'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW this one doesn't need the Theme
parameter to be passed manually: https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/Chip/Chip.types.ts#L78
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With this change, I should Omit
the sx
props from Paper
as follows:
export interface SnackbarContentProps extends Omit<StandardProps<PaperProps, 'children'>, 'sx'>
Is it ok?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes ~ it makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that 'sx' has been passed to Paper
and that mui-material
is used for Paper, we are required to use the Paper 'sx' type until Paper is migrated to material-next.
I have added a TODO for this section.
packages/mui-material-next/src/SnackbarContent/SnackbarContent.types.ts
Outdated
Show resolved
Hide resolved
65d4200
to
24b3794
Compare
24b3794
to
5ad7d52
Compare
Hi @mj12albert, |
Hey ~ I was out for several days, will review the latest changes in the next couple of days! |
})} | ||
> | ||
|
||
{data.length > 0 && ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Best-Sardar Are the changes to this file still necessary?
flexWrap: 'wrap', | ||
alignItems: 'center', | ||
padding: '6px 16px', | ||
borderRadius: (theme.vars || theme).shape.borderRadius, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
borderRadius: (theme.vars || theme).shape.borderRadius, | |
borderRadius: tokens.shape.borderRadius, |
const emphasis = theme.palette.mode === 'light' ? 0.8 : 0.98; | ||
const backgroundColor = emphasize(theme.palette.background.default, emphasis); | ||
})<{ ownerState: SnackbarContentOwnerState }>(({ theme }) => { | ||
const { vars: tokens } = theme; | ||
|
||
return { | ||
...theme.typography.body2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Best-Sardar We can break up this spread to use theme.vars
as well, you can use https://github.com/mui/material-ui/blob/master/packages/mui-material-next/src/FormLabel/FormLabel.tsx#L49-L53 as a reference
additionalProps: { | ||
ref, | ||
}, | ||
additionalProps: { ref }, | ||
className: [classes.root, className], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
className: [classes.root, className], | |
className: [classes.root], |
We don't need to manually destructure this className
from props and pass it here anymore, it's ok to leave it in other
and let it get passed to externalForwardedProps
if you are not doing anything else with it, see #39601 for background!
marginRight: -8, | ||
}); | ||
|
||
const SnackbarContent = React.forwardRef(function SnackbarContent(inProps, ref) { | ||
const SnackbarContent = React.forwardRef(function SnackbarContent( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Best-Sardar Let's implement useSlotProps
here for the three slots: root
, message
and action
!
Closing as stale. This implementation of MD3 is one that we probably won't follow. |
related to: #39207
In this PR, I have converted the Snackbar files from JavaScript to TypeScript.
Furthermore, to apply the Material Design 3 specs, I also need to migrate the SnackbarContent component to mui-material-next. I will submit the changes for SnackbarContent in a separate PR.