-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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][ButtonGroup] Copy ButtonGroup to material next #39716
[material-next][ButtonGroup] Copy ButtonGroup to material next #39716
Conversation
Netlify deploy previewhttps://deploy-preview-39716--material-ui.netlify.app/ Bundle size reportDetails of bundle changes (Toolpad) |
I think the errors in the tests are caused by the different ButtonGroupContexts in the Button and the ButtonGroup. I would suggest to delete the .test file for this PR and add it again in a later PR when the new ButtonGroupContext is implemented in the Button and ButtonGroup of |
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.
Hey @lhilgert9, thanks for working on this 🎉
About the failing tests, I think these imports should be changed, would that fix it?
import * as React from 'react'; | ||
import { expect } from 'chai'; | ||
import { createRenderer, describeConformance, screen } from '@mui-internal/test-utils'; | ||
import ButtonGroup, { buttonGroupClasses as classes } from '@mui/material/ButtonGroup'; |
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 should be imported from @mui/material-next/ButtonGroup
import { createRenderer, describeConformance, screen } from '@mui-internal/test-utils'; | ||
import ButtonGroup, { buttonGroupClasses as classes } from '@mui/material/ButtonGroup'; | ||
import { ThemeProvider, createTheme } from '@mui/material/styles'; | ||
import Button, { buttonClasses } from '@mui/material/Button'; |
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 should be imported from @mui/material-next/Button
Answering to myself: it probably breaks other tests because these changes #38520 need to be ported to |
No, the problem is, that the Button in material-next is not implementing the ButtonGroupContext, so the test would fail, because the props are not provided to the buttons. |
Let's implement ButtonGroupContext in |
So you want to continue in this PR because I've already completely done this work to the other PR. As I said, it only depends on the error that occurs during the test. Maybe we could also just try to find the error in PR #39699 |
Yes ~ we'll be able to review and iterate much faster than one big PR! |
Replaced by #39739 |
ButtonGroup issue: #39686
Material You umbrella issue: #29345
Changes
material
v5