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

[material-next][Snackbar] apply MD3 designs #39631

Closed

Conversation

Best-Sardar
Copy link
Contributor

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.

@mui-bot
Copy link

mui-bot commented Oct 27, 2023

Netlify deploy preview

https://deploy-preview-39631--material-ui.netlify.app/

@mui/material-next: parsed: +1.38% , gzip: +1.24%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against 5ad7d52

@Best-Sardar Best-Sardar force-pushed the material-you-add-snackbar-component branch from 2a9538c to 3dea297 Compare October 27, 2023 12:43
@zannager zannager added the component: snackbar This is the name of the generic UI component, not the React module! label Oct 27, 2023
@samuelsycamore samuelsycamore changed the title [Snackbar][Material You] convert .js files to .ts [material-next][Snackbar] convert .js files to .ts Oct 28, 2023
@ZeeshanTamboli
Copy link
Member

I'd like @DiegoAndai to review this. I am not aware of the Snackbar MD3 plan.

@ZeeshanTamboli ZeeshanTamboli requested review from DiegoAndai and removed request for ZeeshanTamboli October 30, 2023 08:34
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Oct 30, 2023
@DiegoAndai
Copy link
Member

@Best-Sardar is this ready for review?

@Best-Sardar
Copy link
Contributor Author

Best-Sardar commented Nov 1, 2023

Hi @DiegoAndai,
In this PR, I have only converted the files to TypeScript (ts). I needed SnackbarContent to implement the MD3 designs, which was separated into a separate PR.
I can draft this PR and send you all the necessary changes for implementing the MD3 designs in this PR in different commits.
which one do you prefere?

@Best-Sardar Best-Sardar force-pushed the material-you-add-snackbar-component branch from 3dea297 to c4f19f3 Compare November 2, 2023 11:39
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 2, 2023
@Best-Sardar
Copy link
Contributor Author

image

Hi @DiegoAndai,
Considering the design of the action button in MD3 and the fact that these actions are sent from outside to the component as actions, what approach do you suggest for implementing the designs of material?
i need to change the action button colors to inverse-primary

@DiegoAndai
Copy link
Member

In this PR, I have only converted the files to TypeScript

That's good 👍🏼 let's keep this PR only for TypeScript and have a separate one for MD3 design.

Considering the design of the action button in MD3 and the fact that these actions are sent from outside to the component as actions, what approach do you suggest for implementing the designs of material?

I didn't understand the question. The Snackbar's button is the text Button variant, right?

@Best-Sardar
Copy link
Contributor Author

image

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).
The question I have is how should I proceed to implement the correct color for the action button? It seems to me that we can assign a specific color to the button for the inverse such as color="inversePrimary" (as per the Figma designs, this color is only used in this case).

Actually, the actions button are not text button with primary color.
@DiegoAndai

@DiegoAndai
Copy link
Member

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).

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 inverse-primary color to the theme though, could you do that?

Regarding this Typescript PR, should we review it already?

@Best-Sardar Best-Sardar force-pushed the material-you-add-snackbar-component branch from 0a1090b to 65d4200 Compare November 3, 2023 19:44
@Best-Sardar
Copy link
Contributor Author

Hi @DiegoAndai,
Considering the relatively small size of the changes required to implement the MD3 design, I have sent you the rest changes in this PR in deferent commits. Since the changes are interdependent, I believe it would be easier for you to review them together.

There are a few things I need to tell you:

  1. To create the SnackBar documentation (Experimental APIs section), it was necessary to hide the playground section (there was nothing there). I tried to remove that section from the display with minimal changes (if you agree, I can perform some refactoring on this component). In GitHub, there are many changes displayed, but in reality, I added a condition data.length > 0 for the playground section only.

  2. For the onClose type in the SnackBar, it was necessary to add null because this value was being passed to onClose as a reason during autoHide. This change affected other parts besides material-next.

The overall changes have been made and are ready for review.
If you prefer, I can create separate PRs for each change.

@mj12albert mj12albert self-assigned this Nov 7, 2023
@mj12albert mj12albert self-requested a review November 7, 2023 07:26
@@ -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';
Copy link
Member

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 👀

Copy link
Member

@mj12albert mj12albert left a 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';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { SxProps } from '@mui/system';
import { SxProps } from '../styles/Theme.types';

Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes ~ it makes sense

Copy link
Contributor Author

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/Snackbar/Snackbar.types.ts Outdated Show resolved Hide resolved
@Best-Sardar Best-Sardar force-pushed the material-you-add-snackbar-component branch from 65d4200 to 24b3794 Compare November 9, 2023 15:16
@Best-Sardar Best-Sardar force-pushed the material-you-add-snackbar-component branch from 24b3794 to 5ad7d52 Compare November 10, 2023 13:11
@Best-Sardar Best-Sardar changed the title [material-next][Snackbar] convert .js files to .ts [material-next][Snackbar] apply MD3 designs Nov 16, 2023
@Best-Sardar
Copy link
Contributor Author

Hi @mj12albert,
Could you give me an update about this PR?

@mj12albert
Copy link
Member

Hi @mj12albert, Could you give me an update about this PR?

Hey ~ I was out for several days, will review the latest changes in the next couple of days!

})}
>

{data.length > 0 && (
Copy link
Member

@mj12albert mj12albert Nov 20, 2023

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,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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,
Copy link
Member

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],
Copy link
Member

@mj12albert mj12albert Nov 20, 2023

Choose a reason for hiding this comment

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

Suggested change
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(
Copy link
Member

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!

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 3, 2024
@DiegoAndai
Copy link
Member

Closing as stale. This implementation of MD3 is one that we probably won't follow.

@DiegoAndai DiegoAndai closed this Dec 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: snackbar This is the name of the generic UI component, not the React module! PR: out-of-date The pull request has merge conflicts and can't be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants