-
-
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-ui][ToggleButtonGroup] Support different elements under it #40220
[material-ui][ToggleButtonGroup] Support different elements under it #40220
Conversation
Netlify deploy previewhttps://deploy-preview-40220--material-ui.netlify.app/ @material-ui/core: parsed: +0.19% , gzip: +0.17% Bundle size reportDetails of bundle changes (Toolpad) |
3159eb9
to
2bcadc1
Compare
2bcadc1
to
3e27ea1
Compare
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.
After quickly reviewing the implementation, it seems satisfactory. However, could you include tests to confirm that the changes are functional? Specifically, test scenarios involving Toggle buttons wrapped with tooltips, ensuring that a toggle button is selected, and that styles are maintained when tooltips are wrapped around disabled buttons with span
.
Also, I believe it closes both #12921 and #36270, so can you update the description to automatically close it when merged?
I wasn't sure if #12921 was just about ToggleButtonGroup or if it's about replacing usages of |
0867d77
to
4a7617d
Compare
I added the changes similar to #38520 and #38989 to fix the styling for varying children and added some tests as well. The current issue is that sometimes the CSS gets overridden, by either |
Alright, I overcame that issue by using a more specific selector to make sure the toggle button border styling is preserved even when the button is disabled. Everything should be ready to go, let me know if there's anything else you need! |
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.
I wasn't sure if #12921 was just about ToggleButtonGroup or if it's about replacing usages of cloneElement more broadly, do you still want me to mark this PR as closing that issue?
After reviewing the issue thread, I can confirm it is related to the toggle button group. We can mark the PR as resolving that issue.
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupButtonContext.ts
Outdated
Show resolved
Hide resolved
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupContext.ts
Outdated
Show resolved
Hide resolved
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroup.js
Outdated
Show resolved
Hide resolved
'& .MuiToggleButtonGroup-middleButton': { | ||
marginLeft: -1, | ||
borderLeft: '1px solid transparent', | ||
}, | ||
'& .MuiToggleButtonGroup-lastButton': { | ||
marginLeft: -1, | ||
borderLeft: '1px solid transparent', |
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.
We can combine this:
'& .MuiToggleButtonGroup-middleButton': { | |
marginLeft: -1, | |
borderLeft: '1px solid transparent', | |
}, | |
'& .MuiToggleButtonGroup-lastButton': { | |
marginLeft: -1, | |
borderLeft: '1px solid transparent', | |
'& .MuiToggleButtonGroup-middleButton,& .MuiToggleButtonGroup-lastButton': { | |
marginLeft: -1, | |
borderLeft: '1px solid transparent', | |
}, |
Now, would this be seen as a breaking change for developers who use the pseudo-classes (:first-of-type, :last-of-type) for custom styling?
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, I believe it is a slight breaking change, because the pseudo-classes were more specific than the new classes, and therefore took higher precedence. I think #38520 was also a slight breaking change in that sense, since this PR borrows this strategy from that PR?
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.
I believe merging this PR is worthwhile for resolving the associated issue/s (considering the upvotes in #12921), even if it involves a minor risk of breaking change. Given our successful inclusion of #38520 in a previous patch release without any issues, I think we can do the same with this one.
My only concern is that developers who have copied this demo directly into their projects may be affected by the changes in this PR.
Let's keep this conversation thread open even after the PR gets merged.
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupContext.ts
Outdated
Show resolved
Hide resolved
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupContext.ts
Outdated
Show resolved
Hide resolved
packages/mui-material/src/ToggleButtonGroup/ToggleButtonGroupContext.ts
Outdated
Show resolved
Hide resolved
packages/mui-material/src/ToggleButtonGroup/toggleButtonGroupClasses.ts
Outdated
Show resolved
Hide resolved
…ontext.ts Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Nathan Bierema <nbierema@gmail.com>
….tsx Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Nathan Bierema <nbierema@gmail.com>
…ontext.ts Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Nathan Bierema <nbierema@gmail.com>
…ontext.ts Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Nathan Bierema <nbierema@gmail.com>
…lasses.ts Co-authored-by: Zeeshan Tamboli <zeeshan.tamboli@gmail.com> Signed-off-by: Nathan Bierema <nbierema@gmail.com>
PR review comments addressed. |
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.
Looks good! Thanks for the contribution!
…ui#40220) Signed-off-by: Nathan Bierema <nbierema@gmail.com> Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
Uses context for passing props from
ToggleButtonGroup
toToggleButton
s in the vein of #28645.Fixes #36270
Fixes #12921
UI Regression test component image: