[material-ui][ButtonGroup] Deprecate composed classes #41259
Conversation
sai6855
added
docs
Netlify deploy previewBundle size reportDetails of bundle changes (Toolpad) |
| 'horizontal', | ||
| 'vertical', | ||
| 'colorPrimary', | ||
| 'colorSecondary', |
| it('should have groupedColorPrimary classe', () => { | ||
| const { container } = render( | ||
| <ButtonGroup> | ||
| <Button>Hello World</Button> | ||
| </ButtonGroup>, | ||
| ); | ||
| const buttonGroup = container.firstChild; | ||
| expect(buttonGroup).to.have.class(classes.colorPrimary); | ||
| expect(buttonGroup).to.have.class(classes.horizontal); | ||
| }); | ||
|
|
||
| it('should have groupedSecondary classe', () => { | ||
| const { container } = render( | ||
| <ButtonGroup color="secondary"> | ||
| <Button>Hello World</Button> | ||
| </ButtonGroup>, | ||
| ); | ||
|
|
||
| const buttonGroup = container.firstChild; | ||
| expect(buttonGroup).to.have.class(classes.colorSecondary); | ||
| }); | ||
|
|
There was a problem hiding this comment.
tests for new classes
|
|
sai6855
force-pushed
the
deprecate-button-group-classes
branch
from
0a34aaf
to
55c9e10
Compare
| }); | ||
| }); | ||
|
|
||
| const selector = `${replacementSelectorPrefix}${deprecatedClass}`; |
There was a problem hiding this comment.
Observed an issue with regex approach here (the approach which already merged PR's are using).
when running codemod for .MuiButtonGroup-groupedTextHorizontal regex matched with .MuiButtonGroup-groupedText and applied replacementSelector of .MuiButtonGroup-groupedText which is .MuiButtonGroup-text > .MuiButtonGroup-grouped instead of applying replaceSelector of .MuiButtonGroup-groupedTextHorizontal.
So went with absolute equality approach instead of regex equality
There was a problem hiding this comment.
The problem with absolute equality is that it won't match when the literal has more classes, for example .Mui-disabled .MuiButtonGroup-groupedTextHorizontal. We could use regex's $ matcher to match the end of the file, or a dot . which would denote another class.
new Regex(`${replacementSelectorPrefix}${deprecatedClass}(.|$)`)Would that work?
There was a problem hiding this comment.
The problem with absolute equality is that it won't match when the literal has more classes, for example .Mui-disabled .MuiButtonGroup-groupedTextHorizontal. We could use regex's $ matcher to match the end of the file, or a dot . which would denote another class.
i see, got it.
new RegExp(
${replacementSelectorPrefix}${deprecatedClass}(.|$))
This regexp was transforming ('& .MuiButtonGroup-groupedTextHorizontal'); to ("&.MuiButtonGroup-text > .MuiButtonGroup-groupedorizontal"); instead of ("&.MuiButtonGroup-text.MuiButtonGroup-horizontal > .MuiButtonGroup-grouped");
so changed Regexp to new RegExp(${replacementSelectorPrefix}${deprecatedClass}($)) and it seems like this is working
There was a problem hiding this comment.
I'm sorry, my mistake. We must escape the dot as it's a regex symbol for any character. This regex should work:
new Regex(`${replacementSelectorPrefix}${deprecatedClass}(\.| |$)`)I added the space as well " " to accommodate for other selectors.
Let me know if that works and sorry for the late reply.
There was a problem hiding this comment.
Tried
new RegExp(`${replacementSelectorPrefix}${deprecatedClass}(\.| |$)`)and
new RegExp(`${replacementSelectorPrefix}${deprecatedClass}(.| |$)`)both are transforimg ('& .MuiButtonGroup-groupedTextHorizontal'); to ("&.MuiButtonGroup-text > .MuiButtonGroup-groupedorizontal");
There was a problem hiding this comment.
That's weird, do you think it's matching with MuiButtonGroup-groupedText instead? It shouldn't:
Did this one work?:
new RegExp(`${replacementSelectorPrefix}${deprecatedClass}$`)There was a problem hiding this comment.
Did this one work? new RegExp(
${replacementSelectorPrefix}${deprecatedClass}$)
Yes, this worked. commit reference: 67e475f
sai6855
force-pushed
the
deprecate-button-group-classes
branch
from
8d0f26c
to
a17eea0
Compare
sai6855
force-pushed
the
deprecate-button-group-classes
branch
from
a17eea0
to
0a6182b
Compare
There was a problem hiding this comment.
Hey @sai6855, I reviewed and found a couple of details but I think after that this is ready to merge.
Next week, we'll freeze v5 and open the next branch to release v6 alpha versions. Would you mind waiting until then to merge this? This way, we'll avoid fixing any error in v5 that may have slipped. This is to be extra careful as we'll try not to release any v5 patches after it's frozen. The idea is to use this week to merge only urgent that have to be included in v5. Does that make sense?
If you're on board, I'll ask you to wait a bit longer, and when the next branch is created next week, point this PR to that branch instead. I'll let you know.
packages/mui-codemod/src/deprecations/button-group-classes/postcss-plugin.js
Outdated
Show resolved
Hide resolved
part of #41282
classes api doc: https://deploy-preview-41259--material-ui.netlify.app/material-ui/api/button-group/#classes
migration guide: https://deploy-preview-41259--material-ui.netlify.app/material-ui/migration/migrating-from-deprecated-apis/#buttongroup