[ButtonGroup] Support different elements under ButtonGroup #28645

ZeeshanTamboli · 2021-09-27T12:44:23Z

I have followed (at least) the PR section of the contributing guide.

Use Context API instead of React.cloneElement.

I have also considered https://reactjs.org/docs/context.html#caveats by using useMemo. But I have not profiled any performance metrics.

Also let me know if we want to test this in some other ways.

CodeSandbox with the fix.

…t different elements other than <Button />

…ButtonGroup

mui-pr-bot · 2021-09-28T04:49:55Z

Details of bundle changes

@material-ui/lab: parsed: +0.08% , gzip: +0.10%

Generated by 🚫 dangerJS against 1c026b8

PabloLION · 2021-10-05T21:46:48Z

Any news on this PR? I am using a customized <IconButton> as a child of <ButtonGroup>.
I can only convert them to <Button> for now.

ZeeshanTamboli · 2021-10-06T04:48:35Z

Any news on this PR? I am using a customized <IconButton> as a child of <ButtonGroup>. I can only convert them to <Button> for now.

It would help if someone gets time to review this PR.

@Pablion Until then you can use this PR's preview package and check if it works for you.

"@mui/material": "https://pkg.csb.dev/mui-org/material-ui/commit/34759013/@mui/material",

…ButtonGroup

PabloLION · 2021-10-08T12:00:26Z

@ZeeshanTamboli I just checked the new preview with my localhost, and it is improved but not what expacted. I'm trying to create to create a sandbox to explain but they dont support "@mui/material": "https://pkg.csb.dev/mui-org/material-ui/commit/34759013/@mui/material" nor the "@mui/material": "^5.0.0",. Any help?

PabloLION · 2021-10-08T13:57:01Z

I made a explanatory demo with Code Sandbox.

ZeeshanTamboli · 2021-10-11T09:06:22Z

I made a explanatory demo with Code Sandbox.

@Pablion Thanks for providing the codesandbox. You need to pass color="primary" prop to the IconButtons as explained in the doc. By default, the color is default.

See this updated CodeSandbox.

For how it is working on MUI 5.0.3 (the left content), I think anyway the IconButton is not supported inside ButtonGroup and it was wrong. You can check the hovering styles as well in GoogleIcon, looks a bit off.

PabloLION · 2021-10-12T02:44:58Z

Hi,@ZeeshanTamboli,
Sorry for the misguiding example. My intention was to make the source code less changed, and I knew about the color="primary". I just want to point out that

The border style has also changed.
Those components wrapped by <a> and <button> are still considered as different class (shown in first section of 2nd Updated CodeSandbox here).

And I think that should be part of "Support different elements under ButtonGroup"

siriwatknp · 2021-10-19T04:17:21Z

Should it be a demo to showcase the children that are not Button?

packages/mui-material/src/ButtonGroup/ButtonGroupContext.ts

…ButtonGroup

PabloLION · 2021-10-26T09:45:35Z

Is there something new to test with?

packages/mui-material/src/Button/Button.js

…annotation

Comments addressed

ZeeshanTamboli · 2021-10-28T11:01:44Z

All right! There's just two more issues to solve:

Argos shows that the appearance of buttons inside a group changed. Could you make sure there are no differences in existing components?

The CI complains about framer changes not being checked in. Please run yarn workspace framer build locally and commit the changes.

See #28645 (comment)

… default values

oliviertassinari · 2021-10-28T13:18:12Z

packages/mui-material/src/Button/Button.js

    ...other
  } = props;

+  const color = colorProp || colorContext || 'primary';
+  const disabled = disabledProp || disabledContext || false;
+  const disableElevation = disableElevationProp || disableElevationContext || false;


Using || looks wrong here. If disableElevationContext is true and disableElevationProp false, disableElevation should be false, not true.

Otherwise, we could explore the creation of a helper. I assume that it's always the same case when a parent component uses the context to change default values on its children.

Using || looks wrong here. If disableElevationContext is true and disableElevationProp false, disableElevation should be false, not true.

Nice catch. That is interesting! Need to think how to handle these button boolean props if used with context.

I assume this would work:

Suggested change

const disableElevation = disableElevationProp || disableElevationContext || false;

const disableElevation = disableElevationProp ?? disableElevationContext ?? false;

I saw it used by Michal const ButtonRoot: React.ElementType = component ?? components.Root ?? 'button';

@oliviertassinari I'm afraid that his is not supposed to be used here. If i recall it correctly, ?? checks nullity (undefined/null) instead of falsy/truthy.

Example here (i just wrote it)

So if a user has disableElevationProp={false} disableElevationContext={true}, this version with ?? will return a false instead of true. (see the exception_case in that example)

component ?? components.Root ?? 'button' was correctly used or equally components?.Root ?? 'button'.

we do not support it like this without Context

It's interesting no one has filed a bug with this yet. I'd say let's keep the current behavior and fix it for the next major (and in unstyled Button when ButtonGroup is introduced there).

I'd say let's keep the current behavior and fix it for the next major (and in unstyled Button when ButtonGroup is introduced there).

Okay. With that I think there is nothing else pending in this PR. Resolving this conversation.

@ZeeshanTamboli could we add a todo to change this behavior in v6?

I was just writing tests for this and noticed that we do not support it like this without Context.

@ZeeshanTamboli it looks like more bugs.

mnajdova

See one last comment for adding TODO for v6. Apart from that, it looks great! :)

…ButtonGroup

PabloLION · 2021-11-02T07:53:04Z

Hi MUI team. does this close #28899 as well? And will it on next patch?

ZeeshanTamboli added 5 commits September 24, 2021 17:06

replace React.cloneElement with Context API in ButtonGroup for suppor…

38c2624

…t different elements other than <Button />

use ButtonGroupContext in Button component

abdc7e6

test: ButtonGroup context

7bf3f2b

test: ButtonGroup context

b2a5904

yarn prettier

b035d15

ZeeshanTamboli marked this pull request as ready for review September 27, 2021 13:21

ZeeshanTamboli self-assigned this Sep 27, 2021

ZeeshanTamboli marked this pull request as draft September 27, 2021 13:21

ZeeshanTamboli added 2 commits September 28, 2021 10:17

add types for ButtonGroupContext

aa88456

Merge branch 'master' into issue/17226-support-different-elements-to-…

14a6172

…ButtonGroup

ZeeshanTamboli added the component: ButtonGroup The React component. label Sep 28, 2021

ZeeshanTamboli added the new feature New feature or request label Sep 28, 2021

ZeeshanTamboli added 2 commits September 28, 2021 10:35

yarn prettier

3cbe153

yarn workspace framer build

9f6dddb

ZeeshanTamboli marked this pull request as ready for review September 28, 2021 05:40

ZeeshanTamboli requested a review from oliviertassinari September 28, 2021 13:21

aniruddhabagal approved these changes Oct 1, 2021

View reviewed changes

Merge branch 'master' into issue/17226-support-different-elements-to-…

3475901

…ButtonGroup

aniruddhabagal approved these changes Oct 6, 2021

View reviewed changes

github-actions bot mentioned this pull request Oct 6, 2021

Update snapshots for pr/28645 eps1lon/mui-scripts-incubator#3200

Closed

PabloLION mentioned this pull request Oct 8, 2021

How/Why does the props affects the style on ButtonGroup and IconButton #28899

Closed

2 tasks

ZeeshanTamboli requested a review from a team October 15, 2021 04:51

mnajdova mentioned this pull request Oct 25, 2021

ButtonGroup variant outlined always has primary color borders on hover #29219

Closed

2 tasks

mnajdova reviewed Oct 25, 2021

View reviewed changes

packages/mui-material/src/ButtonGroup/ButtonGroupContext.ts Outdated Show resolved Hide resolved

ZeeshanTamboli added 3 commits October 26, 2021 11:56

fix cyclic import

d198923

Merge branch 'master' into issue/17226-support-different-elements-to-…

5989f53

…ButtonGroup

use import type to avoid circular dependency

e1e6657

mnajdova requested a review from michaldudak October 28, 2021 08:21

michaldudak reviewed Oct 28, 2021

View reviewed changes

packages/mui-material/src/Button/Button.js Outdated Show resolved Hide resolved

change destructuring default values syntax to support JSDoc @default …

e255ea2

…annotation

fix(Button): disableRipple default value

8c4ca0d

fix(Button): initialize button props which may have context values or…

f5f4c2a

… default values

oliviertassinari reviewed Oct 28, 2021

View reviewed changes

ZeeshanTamboli requested review from michaldudak, mnajdova and oliviertassinari November 1, 2021 07:42

mnajdova approved these changes Nov 1, 2021

View reviewed changes

ZeeshanTamboli added 3 commits November 2, 2021 11:43

Merge branch 'master' into issue/17226-support-different-elements-to-…

2773bf9

…ButtonGroup

add TODO comment

6b739f4

yarn prettier

1c026b8

ZeeshanTamboli merged commit a0f23c0 into mui:master Nov 2, 2021

ZeeshanTamboli deleted the issue/17226-support-different-elements-to-ButtonGroup branch November 2, 2021 07:23

greglittlefield-wf mentioned this pull request Nov 4, 2021

[ButtonGroup] Children lacking information about position within the parent #29514

Closed

2 tasks

ZeeshanTamboli mentioned this pull request Nov 10, 2021

[Button] Fix disableRipple getting ignored in MuiButtonBase defaultProps #29613

Closed

1 task

brentertz mentioned this pull request Nov 17, 2021

[ButtonGroup] ButtonGroup props are ignored on child Buttons #29741

Closed

2 tasks

siriwatknp mentioned this pull request Nov 18, 2021

[LoadingButton] defaultProps is no longer inherited from MuiButton defaultProps in @mui/lab 5.0.0-alpha.55 version #29740

Closed

2 tasks

oliviertassinari mentioned this pull request Dec 23, 2021

[LoadingButton] Fix variant default prop not inheriting from MuiButton theme slot #30126

Closed

1 task

Methuselah96 mentioned this pull request Dec 17, 2023

[material-ui][ToggleButtonGroup] Support different elements under it #40220

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ButtonGroup] Support different elements under ButtonGroup #28645

[ButtonGroup] Support different elements under ButtonGroup #28645

ZeeshanTamboli commented Sep 27, 2021 •

edited by hbjORbj

Loading

mui-pr-bot commented Sep 28, 2021 •

edited

Loading

PabloLION commented Oct 5, 2021

ZeeshanTamboli commented Oct 6, 2021 •

edited

Loading

PabloLION commented Oct 8, 2021

PabloLION commented Oct 8, 2021

ZeeshanTamboli commented Oct 11, 2021 •

edited

Loading

PabloLION commented Oct 12, 2021

siriwatknp commented Oct 19, 2021

PabloLION commented Oct 26, 2021

ZeeshanTamboli commented Oct 28, 2021

oliviertassinari Oct 28, 2021 •

edited

Loading

oliviertassinari Oct 28, 2021 •

edited

Loading

ZeeshanTamboli Oct 28, 2021

oliviertassinari Oct 28, 2021 •

edited

Loading

PabloLION Oct 28, 2021

michaldudak Oct 29, 2021

ZeeshanTamboli Nov 1, 2021

mnajdova Nov 1, 2021

ZeeshanTamboli Nov 2, 2021

oliviertassinari Nov 10, 2021

mnajdova left a comment

PabloLION commented Nov 2, 2021

	const disableElevation = disableElevationProp \|\| disableElevationContext \|\| false;
	const disableElevation = disableElevationProp ?? disableElevationContext ?? false;

[ButtonGroup] Support different elements under ButtonGroup #28645

[ButtonGroup] Support different elements under ButtonGroup #28645

Conversation

ZeeshanTamboli commented Sep 27, 2021 • edited by hbjORbj Loading

mui-pr-bot commented Sep 28, 2021 • edited Loading

PabloLION commented Oct 5, 2021

ZeeshanTamboli commented Oct 6, 2021 • edited Loading

PabloLION commented Oct 8, 2021

PabloLION commented Oct 8, 2021

ZeeshanTamboli commented Oct 11, 2021 • edited Loading

PabloLION commented Oct 12, 2021

siriwatknp commented Oct 19, 2021

PabloLION commented Oct 26, 2021

ZeeshanTamboli commented Oct 28, 2021

oliviertassinari Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

ZeeshanTamboli Oct 28, 2021

Choose a reason for hiding this comment

oliviertassinari Oct 28, 2021 • edited Loading

Choose a reason for hiding this comment

PabloLION Oct 28, 2021

Choose a reason for hiding this comment

michaldudak Oct 29, 2021

Choose a reason for hiding this comment

ZeeshanTamboli Nov 1, 2021

Choose a reason for hiding this comment

mnajdova Nov 1, 2021

Choose a reason for hiding this comment

ZeeshanTamboli Nov 2, 2021

Choose a reason for hiding this comment

oliviertassinari Nov 10, 2021

Choose a reason for hiding this comment

mnajdova left a comment

Choose a reason for hiding this comment

PabloLION commented Nov 2, 2021

ZeeshanTamboli commented Sep 27, 2021 •

edited by hbjORbj

Loading

mui-pr-bot commented Sep 28, 2021 •

edited

Loading

ZeeshanTamboli commented Oct 6, 2021 •

edited

Loading

ZeeshanTamboli commented Oct 11, 2021 •

edited

Loading

oliviertassinari Oct 28, 2021 •

edited

Loading

oliviertassinari Oct 28, 2021 •

edited

Loading

oliviertassinari Oct 28, 2021 •

edited

Loading