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

[ButtonGroup] Support Child Button variant overwritting #16918

Closed
1 task done
nate-vukovich opened this issue Aug 7, 2019 · 9 comments · Fixed by #16946
Closed
1 task done

[ButtonGroup] Support Child Button variant overwritting #16918

nate-vukovich opened this issue Aug 7, 2019 · 9 comments · Fixed by #16946
Labels
component: ButtonGroup The React component. good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request

Comments

@nate-vukovich
Copy link
Contributor

  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

When creating a ButtonGroup I actually want to implement the group, but have the possibility to overwrite the Button variant in the child.

Current Behavior 😯

Child overwriting isn't supported - ButtonGroup sets the variant to "outlined" by default

Summary 💡

When creating a ButtonGroup I actually want to implement the group, but have the possibility to overwrite the Button variant in the child.

Motivation 🔦

I have 3 buttons in the group and I use a component reducer to check which one of them is currently "selected / active" - but I've checked the component source and since the ButtonGroup sets the variant I'm not able to overwrite the variant of one of the child buttons.

The idea is to have one set to "contained" to use it as am indicator which one is currently selected.

In the end I would / could create a PR.

@oliviertassinari oliviertassinari added the component: ButtonGroup The React component. label Aug 7, 2019
@oliviertassinari
Copy link
Member

@nvwebd This sounds like a similar concern to: #16876.

@nate-vukovich
Copy link
Contributor Author

@oliviertassinari well kind of - this would only change the color but the difference that would be cool to change is the variant - since there's a big difference between those 2 values.

What I'm trying to achieve is basically have 1 of the "contained" and the other two "outlined".

@oliviertassinari
Copy link
Member

What I'm trying to achieve is basically have 1 of the "contained" and the other two "outlined".

I can imagine a use case for it. Have you tried experimenting around?

@oliviertassinari oliviertassinari added the new feature New feature or request label Aug 7, 2019
@mbrookes
Copy link
Member

mbrookes commented Aug 7, 2019

This sounds like a similar concern to: #16876.

@nvwebd The fix would be the same too.

@nate-vukovich
Copy link
Contributor Author

@oliviertassinari yeah I tried a bit - will have to check if I can simply add the styles directly to it and overwrite the styles to have it filled - will report. If not then I'll update the component itself and create a PR.

@mbrookes sure? I checked the component implementation and it seems to me that only the color would be changed. What I wanted is to use the variant prop to have the styles applied / the button to be filled.

@oliviertassinari oliviertassinari added the good first issue Great for first contributions. Enable to learn the contribution process. label Aug 8, 2019
@oliviertassinari
Copy link
Member

The proposal of @mbrookes seems to work correctly:

diff --git a/docs/src/pages/components/buttons/SplitButton.js b/docs/src/pages/components/buttons/SplitButton.js
index 7ddb74ace..31b7ce9fc 100644
--- a/docs/src/pages/components/buttons/SplitButton.js
+++ b/docs/src/pages/components/buttons/SplitButton.js
@@ -41,7 +41,7 @@ export default function SplitButton() {
   return (
     <Grid container>
       <Grid item xs={12} align="center">
-        <ButtonGroup variant="contained" color="primary" ref={anchorRef} aria-label="split button">
+        <ButtonGroup color="primary" ref={anchorRef} aria-label="split button">
           <Button onClick={handleClick}>{options[selectedIndex]}</Button>
           <Button
             color="primary"
diff --git a/packages/material-ui/src/ButtonGroup/ButtonGroup.js b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
index 0798ea1aa..752ea8057 100644
--- a/packages/material-ui/src/ButtonGroup/ButtonGroup.js
+++ b/packages/material-ui/src/ButtonGroup/ButtonGroup.js
@@ -135,7 +135,7 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(props, ref) {
           disableRipple,
           fullWidth,
           size: child.props.size || size,
-          variant,
+          variant: child.props.variant || variant,
         });
       })}
     </Component>

Capture d’écran 2019-08-08 à 16 15 17
@nvwebd Do you want to submit a pull request? :)

@mbrookes
Copy link
Member

mbrookes commented Aug 8, 2019

@nvwebd Do you want to submit a pull request? :)

Let's not change the current demo though, if that's okay.

@nate-vukovich
Copy link
Contributor Author

Will add the PR then ;)

@nate-vukovich
Copy link
Contributor Author

PR created. Hope I followed correctly :) #16946

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: ButtonGroup The React component. good first issue Great for first contributions. Enable to learn the contribution process. new feature New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants