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-ui][ToggleButtonGroup] Support different elements under it #40220

Merged
merged 19 commits into from Jan 6, 2024

Conversation

Methuselah96
Copy link
Contributor

@Methuselah96 Methuselah96 commented Dec 17, 2023

Uses context for passing props from ToggleButtonGroup to ToggleButtons in the vein of #28645.

Fixes #36270
Fixes #12921


UI Regression test component image:

Screenshot (19)

@mui-bot
Copy link

mui-bot commented Dec 17, 2023

Netlify deploy preview

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

@material-ui/core: parsed: +0.19% , gzip: +0.17%
@material-ui/lab: parsed: +0.40% , gzip: +0.35%

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against fe00b4f

@Methuselah96 Methuselah96 force-pushed the toggle-button-group-context branch 4 times, most recently from 3159eb9 to 2bcadc1 Compare December 17, 2023 04:56
@danilo-leal danilo-leal changed the title [ToggleButtonGroup] Support different elements under ToggleButtonGroup [material-ui][ToggleButtonGroup] Support different elements under it Dec 18, 2023
@danilo-leal danilo-leal added component: toggle button This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Dec 18, 2023
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a 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?

@ZeeshanTamboli ZeeshanTamboli added the PR: needs test The pull request can't be merged label Dec 26, 2023
@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Dec 28, 2023

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?

@Methuselah96
Copy link
Contributor Author

Methuselah96 commented Dec 28, 2023

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 ToggleButton or by custom styles. Any ideas on how to make sure that the CSS defined in ToggleButtonGroup takes precedence in certain cases? The main offending example is that ToggleButton defines a border color for disabled buttons that is overriding the transparent border color defined by ToggleButtonGroup for overlapping buttons.

@Methuselah96
Copy link
Contributor Author

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!

@ZeeshanTamboli ZeeshanTamboli added enhancement This is not a bug, nor a new feature and removed PR: needs test The pull request can't be merged labels Jan 5, 2024
Copy link
Member

@ZeeshanTamboli ZeeshanTamboli left a 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.

Comment on lines 26 to 32
'& .MuiToggleButtonGroup-middleButton': {
marginLeft: -1,
borderLeft: '1px solid transparent',
},
'& .MuiToggleButtonGroup-lastButton': {
marginLeft: -1,
borderLeft: '1px solid transparent',
Copy link
Member

Choose a reason for hiding this comment

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

We can combine this:

Suggested change
'& .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?

Copy link
Contributor Author

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?

Copy link
Member

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.

Methuselah96 and others added 3 commits January 5, 2024 09:07
…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>
Methuselah96 and others added 6 commits January 5, 2024 09:22
…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>
@Methuselah96
Copy link
Contributor Author

PR review comments addressed.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli 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! Thanks for the contribution!

@ZeeshanTamboli ZeeshanTamboli merged commit eb30545 into mui:master Jan 6, 2024
22 checks passed
@Methuselah96 Methuselah96 deleted the toggle-button-group-context branch January 6, 2024 15:16
@ZeeshanTamboli ZeeshanTamboli mentioned this pull request Jan 9, 2024
1 task
mnajdova pushed a commit to mnajdova/material-ui that referenced this pull request Jan 9, 2024
…ui#40220)

Signed-off-by: Nathan Bierema <nbierema@gmail.com>
Co-authored-by: ZeeshanTamboli <zeeshan.tamboli@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: toggle button This is the name of the generic UI component, not the React module! enhancement This is not a bug, nor a new feature package: material-ui Specific to @mui/material
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tooltip on ToggleButton in ToggleButtonGroup Is there a better alternative to cloneElement?
4 participants