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 different elements under ButtonGroup #28645

Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
38c2624
replace React.cloneElement with Context API in ButtonGroup for suppor…
ZeeshanTamboli Sep 24, 2021
abdc7e6
use ButtonGroupContext in Button component
ZeeshanTamboli Sep 27, 2021
7bf3f2b
test: ButtonGroup context
ZeeshanTamboli Sep 27, 2021
b2a5904
test: ButtonGroup context
ZeeshanTamboli Sep 27, 2021
b035d15
yarn prettier
ZeeshanTamboli Sep 27, 2021
aa88456
add types for ButtonGroupContext
ZeeshanTamboli Sep 28, 2021
14a6172
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Sep 28, 2021
3cbe153
yarn prettier
ZeeshanTamboli Sep 28, 2021
9f6dddb
yarn workspace framer build
ZeeshanTamboli Sep 28, 2021
3475901
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Oct 6, 2021
069e626
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Oct 21, 2021
e07da28
code review changes
ZeeshanTamboli Oct 22, 2021
b66656c
yarn prettier
ZeeshanTamboli Oct 22, 2021
3cd743d
test: pass props to ButtonGroup to test if it is received to Context …
ZeeshanTamboli Oct 22, 2021
5fa41eb
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Oct 22, 2021
3770a76
fix cylic import
ZeeshanTamboli Oct 22, 2021
d198923
fix cyclic import
ZeeshanTamboli Oct 26, 2021
5989f53
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Oct 26, 2021
e1e6657
use import type to avoid circular dependency
ZeeshanTamboli Oct 26, 2021
e255ea2
change destructuring default values syntax to support JSDoc @default …
ZeeshanTamboli Oct 28, 2021
8c4ca0d
fix(Button): disableRipple default value
ZeeshanTamboli Oct 28, 2021
f5f4c2a
fix(Button): initialize button props which may have context values or…
ZeeshanTamboli Oct 28, 2021
2773bf9
Merge branch 'master' into issue/17226-support-different-elements-to-…
ZeeshanTamboli Nov 2, 2021
6b739f4
add TODO comment
ZeeshanTamboli Nov 2, 2021
1c026b8
yarn prettier
ZeeshanTamboli Nov 2, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 3 additions & 6 deletions framer/Material-UI.framerfx/code/Button.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ import MuiButton from '@mui/material/Button';
import { Icon } from './Icon';

interface Props {
disabled: boolean;
disableElevation: boolean;
disabled?: boolean;
disableElevation?: boolean;
endIcon: string;
fullWidth: boolean;
fullWidth?: boolean;
href?: string;
startIcon: string;
startIconTheme: 'Filled' | 'Outlined' | 'Rounded' | 'TwoTone' | 'Sharp';
Expand Down Expand Up @@ -44,10 +44,7 @@ export function Button(props: Props): JSX.Element {
}

Button.defaultProps = {
disabled: false,
disableElevation: false,
endIcon: undefined,
fullWidth: false,
startIcon: undefined,
startIconTheme: 'Filled' as 'Filled',
endIconTheme: 'Filled' as 'Filled',
Expand Down
43 changes: 36 additions & 7 deletions packages/mui-material/src/Button/Button.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import useThemeProps from '../styles/useThemeProps';
import ButtonBase from '../ButtonBase';
import capitalize from '../utils/capitalize';
import buttonClasses, { getButtonUtilityClass } from './buttonClasses';
import ButtonGroupContext from '../ButtonGroup/ButtonGroupContext';

const useUtilityClasses = (ownerState) => {
const { color, disableElevation, fullWidth, size, variant, classes } = ownerState;
Expand Down Expand Up @@ -279,23 +280,45 @@ const ButtonEndIcon = styled('span', {

const Button = React.forwardRef(function Button(inProps, ref) {
const props = useThemeProps({ props: inProps, name: 'MuiButton' });
const {
className: classNameContext,
color: colorContext,
disabled: disabledContext,
disableElevation: disableElevationContext,
disableFocusRipple: disableFocusRippleContext,
disableRipple: disableRippleContext,
fullWidth: fullWidthContext,
size: sizeContext,
variant: variantContext,
} = React.useContext(ButtonGroupContext);
const {
children,
color = 'primary',
className,
color: colorProp,
component = 'button',
disabled = false,
disableElevation = false,
disableFocusRipple = false,
disabled: disabledProp,
disableElevation: disableElevationProp,
disableFocusRipple: disableFocusRippleProp,
disableRipple: disableRippleProp,
endIcon: endIconProp,
focusVisibleClassName,
fullWidth = false,
size = 'medium',
fullWidth: fullWidthProp,
size: sizeProp,
startIcon: startIconProp,
type,
variant = 'text',
variant: variantProp,
...other
} = props;

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

@oliviertassinari oliviertassinari Oct 28, 2021

Choose a reason for hiding this comment

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

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

Copy link
Member

@oliviertassinari oliviertassinari Oct 28, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@oliviertassinari oliviertassinari Oct 28, 2021

Choose a reason for hiding this comment

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

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';

Copy link
Contributor

Choose a reason for hiding this comment

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

@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'.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Added.

Copy link
Member

Choose a reason for hiding this comment

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

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.

const disableFocusRipple = disableFocusRippleProp || disableFocusRippleContext || false;
const fullWidth = fullWidthProp || fullWidthContext || false;
const size = sizeProp || sizeContext || 'medium';
const variant = variantProp || variantContext || 'text';
const disableRipple = disableRippleProp || disableRippleContext || false;

const ownerState = {
...props,
color,
Expand Down Expand Up @@ -326,8 +349,10 @@ const Button = React.forwardRef(function Button(inProps, ref) {
return (
<ButtonRoot
ownerState={ownerState}
className={clsx(className, classNameContext)}
component={component}
disabled={disabled}
disableRipple={disableRipple}
focusRipple={!disableFocusRipple}
focusVisibleClassName={clsx(classes.focusVisible, focusVisibleClassName)}
ref={ref}
Expand Down Expand Up @@ -355,6 +380,10 @@ Button.propTypes /* remove-proptypes */ = {
* Override or extend the styles applied to the component.
*/
classes: PropTypes.object,
/**
* @ignore
*/
className: PropTypes.string,
/**
* The color of the component. It supports those theme colors that make sense for this component.
* @default 'primary'
Expand Down
56 changes: 27 additions & 29 deletions packages/mui-material/src/ButtonGroup/ButtonGroup.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import * as React from 'react';
import { isFragment } from 'react-is';
import PropTypes from 'prop-types';
import clsx from 'clsx';
import { unstable_composeClasses as composeClasses } from '@mui/core';
Expand All @@ -8,6 +7,7 @@ import capitalize from '../utils/capitalize';
import styled from '../styles/styled';
import useThemeProps from '../styles/useThemeProps';
import buttonGroupClasses, { getButtonGroupUtilityClass } from './buttonGroupClasses';
import ButtonGroupContext from './ButtonGroupContext';

const overridesResolver = (props, styles) => {
const { ownerState } = props;
Expand Down Expand Up @@ -199,6 +199,31 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(inProps, ref) {

const classes = useUtilityClasses(ownerState);

const context = React.useMemo(
() => ({
className: classes.grouped,
color,
disabled,
disableElevation,
disableFocusRipple,
disableRipple,
fullWidth,
size,
variant,
}),
[
color,
disabled,
disableElevation,
disableFocusRipple,
disableRipple,
fullWidth,
size,
variant,
classes.grouped,
],
);

return (
<ButtonGroupRoot
as={component}
Expand All @@ -208,34 +233,7 @@ const ButtonGroup = React.forwardRef(function ButtonGroup(inProps, ref) {
ownerState={ownerState}
{...other}
>
{React.Children.map(children, (child) => {
if (!React.isValidElement(child)) {
return null;
}

if (process.env.NODE_ENV !== 'production') {
if (isFragment(child)) {
console.error(
[
"MUI: The ButtonGroup component doesn't accept a Fragment as a child.",
'Consider providing an array instead.',
].join('\n'),
);
}
}

return React.cloneElement(child, {
className: clsx(classes.grouped, child.props.className),
color: child.props.color || color,
disabled: child.props.disabled || disabled,
disableElevation: child.props.disableElevation || disableElevation,
disableFocusRipple,
disableRipple,
fullWidth,
size: child.props.size || size,
variant: child.props.variant || variant,
});
})}
<ButtonGroupContext.Provider value={context}>{children}</ButtonGroupContext.Provider>
</ButtonGroupRoot>
);
});
Expand Down
22 changes: 22 additions & 0 deletions packages/mui-material/src/ButtonGroup/ButtonGroup.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { expect } from 'chai';
import { createClientRender, describeConformance } from 'test/utils';
import ButtonGroup, { buttonGroupClasses as classes } from '@mui/material/ButtonGroup';
import Button from '@mui/material/Button';
import ButtonGroupContext from './ButtonGroupContext';

describe('<ButtonGroup />', () => {
const render = createClientRender();
Expand Down Expand Up @@ -148,4 +149,25 @@ describe('<ButtonGroup />', () => {
expect(buttonGroup).to.have.class(classes.fullWidth);
expect(button).to.have.class('MuiButton-fullWidth');
});

it('should forward the context to children', () => {
let context;
render(
<ButtonGroup size="large" variant="contained">
<ButtonGroupContext.Consumer>
{(value) => {
context = value;
}}
</ButtonGroupContext.Consumer>
</ButtonGroup>,
);
expect(context.variant).to.equal('contained');
expect(context.size).to.equal('large');
expect(context.fullWidth).to.equal(false);
expect(context.disableRipple).to.equal(false);
expect(context.disableFocusRipple).to.equal(false);
expect(context.disableElevation).to.equal(false);
expect(context.disabled).to.equal(false);
expect(context.color).to.equal('primary');
});
});
25 changes: 25 additions & 0 deletions packages/mui-material/src/ButtonGroup/ButtonGroupContext.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import * as React from 'react';
import type { ButtonGroupProps } from './ButtonGroup';

interface IButtonGroupContext {
className?: string;
color?: ButtonGroupProps['color'];
disabled?: boolean;
disabledElevation?: boolean;
disableFocusRipple?: boolean;
disableRipple?: boolean;
fullWidth?: boolean;
size?: ButtonGroupProps['size'];
variant?: ButtonGroupProps['variant'];
}

/**
* @ignore - internal component.
*/
const ButtonGroupContext = React.createContext<IButtonGroupContext>({});

if (process.env.NODE_ENV !== 'production') {
ButtonGroupContext.displayName = 'ButtonGroupContext';
}

export default ButtonGroupContext;