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.

Sign up for GitHub

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

Jump to bottom

[material-ui][Button] Deprecate composed classes for v6 #40675

Merged
merged 27 commits into from
Feb 23, 2024
Merged

[material-ui][Button] Deprecate composed classes for v6 #40675

merged 27 commits into from
Feb 23, 2024

Conversation

sai6855
Copy link
Contributor

@sai6855 sai6855 commented Jan 18, 2024 •
edited
Loading

@sai6855 sai6855 added component: button This is the name of the generic UI component, not the React module! deprecation New deprecation message package: material-ui Specific to @mui/material labels Jan 18, 2024
@mui-bot
Copy link

mui-bot commented Jan 18, 2024 •
edited
Loading

Netlify deploy preview

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against ffe5c1e

@sai6855 sai6855 added the v6.x label Jan 18, 2024
@sai6855 sai6855 marked this pull request as draft January 18, 2024 13:03
sai6855
sai6855 commented Jan 18, 2024
View reviewed changes
packages/mui-material/src/Button/buttonClasses.ts
Comment on lines +250 to +255
'colorPrimary',
'colorSecondary',
'colorSuccess',
'colorError',
'colorInfo',
'colorWarning',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new classes

sai6855
sai6855 commented Jan 18, 2024
View reviewed changes
packages/mui-material/src/Button/buttonClasses.ts
@@ -149,6 +268,7 @@ const buttonClasses: ButtonClasses = generateUtilityClasses('MuiButton', [
'fullWidth',
'startIcon',
'endIcon',
'icon',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

new class

@sai6855 sai6855 requested a review from DiegoAndai January 18, 2024 13:12
@sai6855 sai6855 marked this pull request as ready for review January 18, 2024 13:13
@DiegoAndai DiegoAndai mentioned this pull request Jan 19, 2024
Open
@DiegoAndai
Copy link
Member

Hey @sai6855! I'm aware of this PR, but we're discussing whether prop default values should have classes: #40733. Please leave your opinion there if you have one 😊.

So I'll put this on hold until we make a decision. I'll push so we move fast on it.

@DiegoAndai DiegoAndai added the on hold There is a blocker, we need to wait label Jan 22, 2024
@sai6855
Copy link
Contributor Author

sai6855 commented Jan 22, 2024

Got it!! thanks for the update

@sai6855
Copy link
Contributor Author

sai6855 commented Jan 22, 2024

@DiegoAndai since option 1 is choosen for above RFC, I'm assuming no change is required in this PR

@DiegoAndai
Copy link
Member

@sai6855 correct! No change is required.

We'll keep this PR on hold for a little longer as per this comment: #40417 (comment)

@DiegoAndai
Copy link
Member

Hey @sai6855, we're now unblocked: #40417 (comment)

@DiegoAndai DiegoAndai removed the on hold There is a blocker, we need to wait label Feb 15, 2024
@sai6855 sai6855 marked this pull request as draft February 18, 2024 11:34
@sai6855 sai6855 marked this pull request as draft February 20, 2024 16:21
@sai6855 sai6855 removed the request for review from DiegoAndai February 20, 2024 16:21
DiegoAndai
DiegoAndai reviewed Feb 20, 2024
View reviewed changes
packages/mui-codemod/src/deprecations/button-classes/postcss-plugin.js Outdated
Comment on lines 159 to 160
replacementSelector: '.MuiButton-icon.MuiButton-sizeMedium',
replacementSelectorPrefix: '& .',
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't the replacement be &.MuiButton-sizeMedium .MuiButton-icon?

Copy link
Contributor Author

@sai6855 sai6855 Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

from my understanding, iconSizeMedium is split into icon and sizeMedium, both these classes are applied to icon slot and not root slot. But your suggestion seems to add sizeMedium class to root slot. Am i right or did i misunderstood your question?

Copy link
Member

Choose a reason for hiding this comment

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

The size classes are applied to the root: https://mui.com/material-ui/api/button/#button-classes-sizeLarge

So to replace the .MuiButton-iconSizeMedium class, we need to select .MuiButton-sizeMedium .MuiButton-icon: An element with class MuiButton-icon within an element with class MuiButton-sizeMedium. We might want to use the direct child selector .MuiButton-sizeMedium > .MuiButton-icon.

@sai6855 sai6855 marked this pull request as ready for review February 20, 2024 17:30
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 20, 2024
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Feb 21, 2024
sai6855
sai6855 commented Feb 21, 2024
View reviewed changes
packages/mui-codemod/src/deprecations/button-classes/test-cases/expected.js Outdated
[`&.${buttonClasses.sizeMedium}${" > "}.${buttonClasses.icon}`]: {
color: 'red',
},
[`&.${buttonClasses.sizeLarge}${" > "}.${buttonClasses.icon}`]: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried transforming to &.${buttonClasses.sizeLarge} > .${buttonClasses.icon} but couldn't succeed. Does this tranformtion works or shall try again?

Copy link
Member

@DiegoAndai DiegoAndai Feb 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It should work if you add ' > ' to the quasis (here) instead of the expressions when modifying the template literal. The quasis are the chunks of the template string that are not within ${}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, PR is ready for review

@sai6855 sai6855 requested a review from DiegoAndai February 21, 2024 12:17
DiegoAndai
DiegoAndai reviewed Feb 22, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Looks good @sai6855!

Only have two comments left

packages/mui-codemod/src/deprecations/button-classes/button-classes.test.js Show resolved Hide resolved
packages/mui-codemod/src/deprecations/button-classes/test-cases/actual.js Outdated Show resolved Hide resolved
@sai6855 sai6855 requested a review from DiegoAndai February 23, 2024 05:59
DiegoAndai
DiegoAndai approved these changes Feb 23, 2024
View reviewed changes
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Great job @sai6855!

@DiegoAndai DiegoAndai merged commit 8725a44 into mui:master Feb 23, 2024
22 checks passed
@sai6855 sai6855 deleted the deprecate-v6-class-tab branch February 23, 2024 17:16
@oliviertassinari oliviertassinari changed the title [material-ui][Button] Deprecate classes for v6 [material-ui][Button] Deprecate composed classes for v6 Feb 25, 2024
@DiegoAndai DiegoAndai mentioned this pull request Feb 26, 2024
Open
30 tasks
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Mar 8, 2024
@sai6855 @mnajdova
[material-ui][Button] Deprecate classes for v6 (mui#40675)
268ae84
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DiegoAndai DiegoAndai DiegoAndai approved these changes

Assignees
No one assigned
Labels
component: button This is the name of the generic UI component, not the React module! deprecation New deprecation message package: material-ui Specific to @mui/material v6.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sai6855 @mui-bot @DiegoAndai