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

[Button][base] Drop component prop #36677

Merged
merged 36 commits into from
Apr 26, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
c955845
[ButtonUnstyled] Drop `component` prop
mnajdova Mar 29, 2023
8e7ebda
docs:typescript:formatted
mnajdova Mar 29, 2023
615cfaf
Fix lint issues
mnajdova Mar 29, 2023
3b88d2d
Fix tests related to the changes of the conformance test suite
mnajdova Mar 29, 2023
42c3a54
Use React.ButtonHTMLAttributes
mnajdova Mar 29, 2023
abb7d98
remove ButtonUnstyledTypeMap usages
mnajdova Mar 29, 2023
7eecd8a
Merge branch 'master' into base/remove-component-prop
michaldudak Apr 14, 2023
aced6ee
Created a Base UI version of OverridableComponent
michaldudak Apr 17, 2023
b96cc6f
Merge remote-tracking branch 'upstream/master' into base/remove-compo…
michaldudak Apr 17, 2023
1aca46b
revert some test changes, cleanup code
mnajdova Apr 18, 2023
ee4fdb7
Rename the OverridableComponent to PolymorphicComponent
mnajdova Apr 18, 2023
f49956d
Fixed regressions in names
mnajdova Apr 18, 2023
c709d0c
update the spec tests
mnajdova Apr 18, 2023
d61f431
Fix lint issues
mnajdova Apr 19, 2023
d86107a
Update packages/mui-base/src/ButtonUnstyled/ButtonUnstyled.spec.tsx
mnajdova Apr 19, 2023
e0cc64d
Docs updates on the genric usage
mnajdova Apr 19, 2023
10cca14
update button and overriding component structure docs
samuelsycamore Apr 19, 2023
83aaa1e
Merge branch 'master' into base/remove-component-prop
mnajdova Apr 25, 2023
aa50cdc
Merge branch 'base/remove-component-prop' of https://github.com/mnajd…
mnajdova Apr 25, 2023
0c78312
prettier
mnajdova Apr 25, 2023
5c8d684
docs:api
mnajdova Apr 25, 2023
641bd26
fix some issues
mnajdova Apr 25, 2023
27e0da1
fix wrong path separators
mnajdova Apr 25, 2023
cb62291
One more fix
mnajdova Apr 25, 2023
4ebe89f
more changes
mnajdova Apr 25, 2023
41845e6
Remove occurrences of unstyled from button doc
hbjORbj Apr 25, 2023
814df7c
update button api docs
hbjORbj Apr 25, 2023
253450a
remove old files
mnajdova Apr 25, 2023
8c7a61e
Merge branch 'base/remove-component-prop' of https://github.com/mnajd…
mnajdova Apr 25, 2023
920a682
address comments
hbjORbj Apr 25, 2023
508a008
use single quote
hbjORbj Apr 25, 2023
2bb9245
prettier
hbjORbj Apr 25, 2023
fbfbd3c
remove unstyled from overriding component structure doc
hbjORbj Apr 26, 2023
9ce855e
add 1 more ts test
hbjORbj Apr 26, 2023
e089918
resolve merge conflict
hbjORbj Apr 26, 2023
b7d62fb
Merge branch 'master' into base/remove-component-prop
hbjORbj Apr 26, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/data/base/components/button/UnstyledButtonCustom.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ ButtonRoot.propTypes = {
};

const SvgButton = React.forwardRef(function SvgButton(props, ref) {
return <ButtonUnstyled {...props} component={CustomButtonRoot} ref={ref} />;
return <ButtonUnstyled {...props} slots={{ root: CustomButtonRoot }} ref={ref} />;
});

export default function UnstyledButtonCustom() {
Expand Down
2 changes: 1 addition & 1 deletion docs/data/base/components/button/UnstyledButtonCustom.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const SvgButton = React.forwardRef(function SvgButton(
props: ButtonUnstyledProps,
ref: React.ForwardedRef<any>,
) {
return <ButtonUnstyled {...props} component={CustomButtonRoot} ref={ref} />;
return <ButtonUnstyled {...props} slots={{ root: CustomButtonRoot }} ref={ref} />;
});

export default function UnstyledButtonCustom() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ import Stack from '@mui/material/Stack';
export default function UnstyledButtonsDisabledFocusCustom() {
return (
<Stack spacing={2}>
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }} disabled>
focusableWhenDisabled = false
</CustomButton>
<CustomButton component="span" disabled focusableWhenDisabled>
<CustomButton slots={{ root: 'span' }} disabled focusableWhenDisabled>
focusableWhenDisabled = true
</CustomButton>
</Stack>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,15 @@ import ButtonUnstyled, {
} from '@mui/base/ButtonUnstyled';
import { styled } from '@mui/system';
import Stack from '@mui/material/Stack';
import { OverridableComponent } from '@mui/types';
import { OverridableComponent } from '@mui/base/utils';

export default function UnstyledButtonsDisabledFocusCustom() {
return (
<Stack spacing={2}>
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }} disabled>
focusableWhenDisabled = false
</CustomButton>
<CustomButton component="span" disabled focusableWhenDisabled>
<CustomButton slots={{ root: 'span' }} disabled focusableWhenDisabled>
focusableWhenDisabled = true
</CustomButton>
</Stack>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }} disabled>
focusableWhenDisabled = false
</CustomButton>
<CustomButton component="span" disabled focusableWhenDisabled>
<CustomButton slots={{ root: 'span' }} disabled focusableWhenDisabled>
focusableWhenDisabled = true
</CustomButton>
4 changes: 2 additions & 2 deletions docs/data/base/components/button/UnstyledButtonsSpan.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ import Stack from '@mui/material/Stack';
export default function UnstyledButtonsSpan() {
return (
<Stack spacing={2} direction="row">
<CustomButton component="span">Button</CustomButton>
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }}>Button</CustomButton>
<CustomButton slots={{ root: 'span' }} disabled>
Disabled
</CustomButton>
</Stack>
Expand Down
6 changes: 3 additions & 3 deletions docs/data/base/components/button/UnstyledButtonsSpan.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@ import ButtonUnstyled, {
} from '@mui/base/ButtonUnstyled';
import { styled } from '@mui/system';
import Stack from '@mui/material/Stack';
import { OverridableComponent } from '@mui/types';
import { OverridableComponent } from '@mui/base/utils';

export default function UnstyledButtonsSpan() {
return (
<Stack spacing={2} direction="row">
<CustomButton component="span">Button</CustomButton>
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }}>Button</CustomButton>
<CustomButton slots={{ root: 'span' }} disabled>
Disabled
</CustomButton>
</Stack>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
<CustomButton component="span">Button</CustomButton>
<CustomButton component="span" disabled>
<CustomButton slots={{ root: 'span' }}>Button</CustomButton>
<CustomButton slots={{ root: 'span' }} disabled>
Disabled
</CustomButton>
8 changes: 4 additions & 4 deletions docs/data/base/components/button/button.md
Original file line number Diff line number Diff line change
Expand Up @@ -61,10 +61,10 @@ The following props are available on all non-utility Base components.
See [Usage](/base/getting-started/usage/) for full details.
:::

Use the `component` prop to override the root slot with a custom element:
Use the `slots.root` prop to override the root slot with a custom element:

```jsx
<ButtonUnstyled component="div" />
<ButtonUnstyled slots={{ root: 'div' }} />
```

If you provide a non-interactive element such as a `<span>`, the Unstyled Button component will automatically add the necessary accessibility attributes.
Expand All @@ -74,8 +74,8 @@ Compare the attributes on the `<span>` in this demo with the Button from the pre
{{"demo": "UnstyledButtonsSpan.js"}}

:::warning
If an Unstyled Button is customized with a non-button element (i.e. `<ButtonUnstyled component="span" />`), it will not submit the form it's in when clicked.
Similarly, `<ButtonUnstyled component="span" type="reset">` will not reset its parent form.
If an Unstyled Button is customized with a non-button element (i.e. `<ButtonUnstyled slots={{ root: "span" }} />`), it will not submit the form it's in when clicked.
Similarly, `<ButtonUnstyled slots={{ root: "span" }} type="reset">` will not reset its parent form.
:::

## Hook
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ import * as React from 'react';
import ButtonUnstyled from '@mui/base/ButtonUnstyled';

export default function DivButton() {
return <ButtonUnstyled component="div">Button</ButtonUnstyled>;
return <ButtonUnstyled slots={{ root: 'div' }}>Button</ButtonUnstyled>;
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,5 @@ import * as React from 'react';
import ButtonUnstyled from '@mui/base/ButtonUnstyled';

export default function DivButton() {
return <ButtonUnstyled component="div">Button</ButtonUnstyled>;
return <ButtonUnstyled slots={{ root: 'div' }}>Button</ButtonUnstyled>;
}
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<ButtonUnstyled component="div">Button</ButtonUnstyled>
<ButtonUnstyled slots={{ root: 'div' }}>Button</ButtonUnstyled>
1 change: 0 additions & 1 deletion docs/pages/base/api/button-unstyled.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
"description": "func<br>&#124;&nbsp;{ current?: { focusVisible: func } }"
}
},
"component": { "type": { "name": "elementType" } },
"disabled": { "type": { "name": "bool" }, "default": "false" },
"focusableWhenDisabled": { "type": { "name": "bool" }, "default": "false" },
"slotProps": {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
"componentDescription": "The foundation for building custom-styled buttons.",
"propDescriptions": {
"action": "A ref for imperative actions. It currently only supports <code>focusVisible()</code> action.",
"component": "The component used for the root node. Either a string to use a HTML element or a component.",
"disabled": "If <code>true</code>, the component is disabled.",
"focusableWhenDisabled": "If <code>true</code>, allows a disabled button to receive focus.",
"slotProps": "The props used for each slot inside the Button.",
Expand Down
12 changes: 7 additions & 5 deletions packages/mui-base/src/ButtonUnstyled/ButtonUnstyled.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -34,19 +34,21 @@ const polymorphicComponentTest = () => {
{/* @ts-expect-error */}
<ButtonUnstyled invalidProp={0} />

<ButtonUnstyled component="a" href="#" />
<ButtonUnstyled slots={{ root: 'a' }} href="#" />

<ButtonUnstyled component={CustomComponent} stringProp="test" numberProp={0} />
<ButtonUnstyled<typeof CustomComponent>
slots={{ root: CustomComponent }}
stringProp="test"
numberProp={0}
/>
{/* @ts-expect-error */}
<ButtonUnstyled component={CustomComponent} />
<ButtonUnstyled<typeof CustomComponent> slots={{ root: CustomComponent }} />

<ButtonUnstyled
component="button"
onClick={(e: React.MouseEvent<HTMLButtonElement>) => e.currentTarget.checkValidity()}
/>

<ButtonUnstyled<'div'>
mnajdova marked this conversation as resolved.
Show resolved Hide resolved
component="div"
ref={(elem) => {
expectType<HTMLDivElement | null, typeof elem>(elem);
}}
Expand Down
22 changes: 14 additions & 8 deletions packages/mui-base/src/ButtonUnstyled/ButtonUnstyled.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -26,11 +26,12 @@ describe('<ButtonUnstyled />', () => {
expectedClassName: buttonUnstyledClasses.root,
},
},
skip: ['componentProp'],
}));

describe('role attribute', () => {
it('is set when the root component is an HTML element other than a button', () => {
const { getByRole } = render(<ButtonUnstyled component="span" />);
const { getByRole } = render(<ButtonUnstyled slots={{ root: 'span' }} />);
expect(getByRole('button')).not.to.equal(null);
});

Expand All @@ -42,7 +43,7 @@ describe('<ButtonUnstyled />', () => {
) => <span role={props.role} ref={ref} />,
);

const { getByRole } = render(<ButtonUnstyled component={WrappedSpan} />);
const { getByRole } = render(<ButtonUnstyled slots={{ root: WrappedSpan }} />);
expect(getByRole('button')).not.to.equal(null);
});

Expand All @@ -54,7 +55,7 @@ describe('<ButtonUnstyled />', () => {
) => <button role={props.role} ref={ref} />,
);

const { getByRole } = render(<ButtonUnstyled component={WrappedButton} />);
const { getByRole } = render(<ButtonUnstyled slots={{ root: WrappedButton }} />);
expect(getByRole('button')).not.to.have.attribute('role');
});
});
Expand Down Expand Up @@ -104,7 +105,7 @@ describe('<ButtonUnstyled />', () => {
describe('as non-button element', () => {
it('can receive focus when focusableWhenDisabled is set', () => {
const { getByRole } = render(
<ButtonUnstyled component="span" focusableWhenDisabled disabled />,
<ButtonUnstyled slots={{ root: 'span' }} focusableWhenDisabled disabled />,
);

const button = getByRole('button');
Expand All @@ -117,7 +118,7 @@ describe('<ButtonUnstyled />', () => {

it('has aria-disabled and tabIndex attributes set', () => {
const { getByRole } = render(
<ButtonUnstyled component="span" focusableWhenDisabled disabled />,
<ButtonUnstyled slots={{ root: 'span' }} focusableWhenDisabled disabled />,
);

const button = getByRole('button');
Expand All @@ -129,7 +130,12 @@ describe('<ButtonUnstyled />', () => {
it('does not respond to user actions when disabled and focused', () => {
const handleClick = spy();
const { getByRole } = render(
<ButtonUnstyled component="span" focusableWhenDisabled disabled onClick={handleClick} />,
<ButtonUnstyled
slots={{ root: 'span' }}
focusableWhenDisabled
disabled
onClick={handleClick}
/>,
);

const button = getByRole('button');
Expand All @@ -155,7 +161,7 @@ describe('<ButtonUnstyled />', () => {
});

it('renders as the element provided in the "component" prop, even with a "href" prop', () => {
const { getByRole } = render(<ButtonUnstyled component="h1" href="#" />);
const { getByRole } = render(<ButtonUnstyled slots={{ root: 'h1' }} href="#" />);
expect(getByRole('heading')).not.to.equal(null);
});

Expand All @@ -172,7 +178,7 @@ describe('<ButtonUnstyled />', () => {
});

it('renders as the element provided in the "component" prop, even with a "to" prop', () => {
const { getByRole } = render(<ButtonUnstyled component="h1" to="#" />);
const { getByRole } = render(<ButtonUnstyled slots={{ root: 'h1' }} to="#" />);
expect(getByRole('heading')).not.to.equal(null);
});

Expand Down
40 changes: 2 additions & 38 deletions packages/mui-base/src/ButtonUnstyled/ButtonUnstyled.tsx
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if the ButtonUnstyled definition shouldn't be function ButtonUnstyled(props: ButtonUnstyledProps, forwardedRef: React.ForwardedRef<any>) (without the generics). This way, we get strongly typed props inside the component definition. Nothing would change with the exported definition as we cast it to PloymorphicComponent anyway.

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 like the explicitness that we have now. We can open a separate issue for this and see what other things I guess. It's anyway internal change

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import * as React from 'react';
import PropTypes from 'prop-types';
import { OverridableComponent } from '@mui/types';
import { OverridableComponent } from '../utils/OverridableComponent';
import composeClasses from '../composeClasses';
import { getButtonUnstyledUtilityClass } from './buttonUnstyledClasses';
import {
Expand Down Expand Up @@ -45,16 +45,9 @@ const ButtonUnstyled = React.forwardRef(function ButtonUnstyled<
const {
action,
children,
component,
disabled,
focusableWhenDisabled = false,
onBlur,
onClick,
onFocus,
onFocusVisible,
onKeyDown,
onKeyUp,
onMouseLeave,
slotProps = {},
slots = {},
...other
Expand Down Expand Up @@ -88,7 +81,7 @@ const ButtonUnstyled = React.forwardRef(function ButtonUnstyled<
const classes = useUtilityClasses(ownerState);

const defaultElement = other.href || other.to ? 'a' : 'button';
const Root: React.ElementType = component ?? slots.root ?? defaultElement;
const Root: React.ElementType = slots.root ?? defaultElement;
const rootProps: WithOptionalOwnerState<ButtonUnstyledRootSlotProps> = useSlotProps({
elementType: Root,
getSlotProps: getRootProps,
Expand Down Expand Up @@ -124,11 +117,6 @@ ButtonUnstyled.propTypes /* remove-proptypes */ = {
* @ignore
*/
children: PropTypes.node,
/**
* The component used for the root node.
* Either a string to use a HTML element or a component.
*/
component: PropTypes.elementType,
/**
* If `true`, the component is disabled.
* @default false
Expand All @@ -143,34 +131,10 @@ ButtonUnstyled.propTypes /* remove-proptypes */ = {
* @ignore
*/
href: PropTypes.string,
/**
* @ignore
*/
onBlur: PropTypes.func,
Copy link
Member Author

Choose a reason for hiding this comment

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

@michaldudak I think we should go the other way around and explicitly define these props in the ButtonUnstyledProps interface, as if a custom component is used, that does not have these props in the props interface, there will be a typescript error, which would be a bug in my opinion.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if I understand you correctly. Why would there be a TS error? We don't actually use these props anywhere. Also in ButtonUnstyled.spec.tsx we have a case with CustomComponent that don't have these defined and it works fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't have with custom component. For e.g. this is failing:

<ButtonUnstyled<typeof CustomComponent>
  slots={{ root: CustomComponent }}
  stringProp="test"
  numberProp={0}
  onClick={() => {}}
/>

Copy link
Member

Choose a reason for hiding this comment

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

OK, I get it now. It's an interesting problem. The ButtonUnstyled doesn't handle (or require) the click (or several other) events, so strictly speaking, it's unnecessary. It's necessary in the slots.root, though, but there is no way of enforcing it.
In other words, even if we had onClick in ButtonUnstyledProps, it would be possible to provide CustomComponent that ignores its onClick prop and effectively create a broken button.

Copy link
Member Author

Choose a reason for hiding this comment

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

Here is a failing test: https://app.circleci.com/pipelines/github/mui/material-ui/95773/workflows/1ec2c63d-7103-4340-b894-909bf873b7a0/jobs/509209

I think we should update the types to add these event handlers.

Copy link
Member

Choose a reason for hiding this comment

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

A couple of thoughts I have about this:

  1. How do we decide which events to define explicitly?
  2. If they are defined explicitly, we will lose polymorphism in their parameters. We'll have to type them as either MouseEventHandler<Element> or MouseEventHandler<HTMLButtonElement>. In either case, another test will fail (the last one from the .spec suite).
  3. If we use this pattern, we should also update all the other components

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 see your point. I've added ts-expect-error directive on the new test, so that in the future, if we decide to handle this, we have some record that it was an intended decision. We can discuss it separately tough in the future.

/**
* @ignore
*/
onClick: PropTypes.func,
/**
* @ignore
*/
onFocus: PropTypes.func,
/**
* @ignore
*/
onFocusVisible: PropTypes.func,
/**
* @ignore
*/
onKeyDown: PropTypes.func,
/**
* @ignore
*/
onKeyUp: PropTypes.func,
/**
* @ignore
*/
onMouseLeave: PropTypes.func,
/**
* The props used for each slot inside the Button.
* @default {}
Expand Down
18 changes: 10 additions & 8 deletions packages/mui-base/src/ButtonUnstyled/ButtonUnstyled.types.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as React from 'react';
import { OverrideProps, Simplify } from '@mui/types';
import { Simplify } from '@mui/types';
import { UseButtonParameters, UseButtonRootSlotProps } from '../useButton';
import { SlotComponentProps } from '../utils';
import { PolymorphicProps } from '../utils/OverridableComponent';

export interface ButtonUnstyledActions {
focusVisible(): void;
Expand Down Expand Up @@ -44,14 +45,15 @@ export interface ButtonUnstyledSlots {
}

export type ButtonUnstyledProps<
D extends React.ElementType = ButtonUnstyledTypeMap['defaultComponent'],
> = OverrideProps<ButtonUnstyledTypeMap<{}, D>, D> & {
component?: D;
};
RootComponent extends React.ElementType = ButtonUnstyledTypeMap['defaultComponent'],
> = PolymorphicProps<ButtonUnstyledTypeMap<{}, RootComponent>, RootComponent>;

export interface ButtonUnstyledTypeMap<P = {}, D extends React.ElementType = 'button'> {
props: P & ButtonUnstyledOwnProps;
defaultComponent: D;
export interface ButtonUnstyledTypeMap<
AdditionalProps = {},
DefaultComponent extends React.ElementType = 'button',
> {
props: AdditionalProps & ButtonUnstyledOwnProps;
defaultComponent: DefaultComponent;
}

export type ButtonUnstyledOwnerState = ButtonUnstyledOwnProps & {
Expand Down