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] Remove label span #26666

Merged
merged 10 commits into from
Jun 11, 2021
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
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
1 change: 0 additions & 1 deletion docs/pages/api-docs/button.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
"styles": {
"classes": [
"root",
"label",
"text",
"textInherit",
"textPrimary",
Expand Down
4 changes: 1 addition & 3 deletions docs/pages/api-docs/loading-button.json
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
"styles": {
"classes": [
"root",
"label",
"text",
"textInherit",
"textPrimary",
Expand Down Expand Up @@ -62,8 +61,7 @@
"loadingIndicatorStart",
"loadingIndicatorEnd",
"endIconLoadingEnd",
"startIconLoadingStart",
"labelLoadingCenter"
"startIconLoadingStart"
],
"globalClasses": { "focusVisible": "Mui-focusVisible", "disabled": "Mui-disabled" },
"name": "MuiLoadingButton"
Expand Down
10 changes: 10 additions & 0 deletions docs/src/pages/guides/migration-v4/migration-v4.md
Original file line number Diff line number Diff line change
Expand Up @@ -598,6 +598,16 @@ You can use the [`moved-lab-modules` codemod](https://github.com/mui-org/materia

You can use the [`button-color-prop` codemod](https://github.com/mui-org/material-ui/tree/HEAD/packages/material-ui-codemod#button-color-prop) for automatic migration.

- `span` element that wraps children has been removed. `label` classKey is also removed.

```diff
<button class="MuiButton-root">
- <span class="MuiButton-label">
children
- </span>
</button>
```

### Chip

- Rename `default` variant to `filled` for consistency.
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/premium-themes/paperbase/Paperbase.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ theme = {
},
MuiButton: {
styleOverrides: {
label: {
root: {
textTransform: 'none',
},
contained: {
Expand Down
2 changes: 1 addition & 1 deletion docs/src/pages/premium-themes/paperbase/Paperbase.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ theme = {
},
MuiButton: {
styleOverrides: {
label: {
root: {
textTransform: 'none',
},
contained: {
Expand Down
4 changes: 0 additions & 4 deletions docs/translations/api-docs/button/button.json
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@
},
"classDescriptions": {
"root": { "description": "Styles applied to the root element." },
"label": {
"description": "Styles applied to {{nodeName}}.",
"nodeName": "the span element that wraps the children"
},
"text": {
"description": "Styles applied to {{nodeName}} if {{conditions}}.",
"nodeName": "the root element",
Expand Down
9 changes: 0 additions & 9 deletions docs/translations/api-docs/loading-button/loading-button.json
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,6 @@
},
"classDescriptions": {
"root": { "description": "Styles applied to the root element." },
"label": {
"description": "Styles applied to {{nodeName}}.",
"nodeName": "the span element that wraps the children"
},
"text": {
"description": "Styles applied to {{nodeName}} if {{conditions}}.",
"nodeName": "the root element",
Expand Down Expand Up @@ -218,11 +214,6 @@
"description": "Styles applied to {{nodeName}} if {{conditions}}.",
"nodeName": "the startIcon element",
"conditions": "<code>loading={true}</code> and <code>loadingPosition=\"start\"</code>"
},
"labelLoadingCenter": {
"description": "Styles applied to {{nodeName}} if {{conditions}}.",
"nodeName": "the label element",
"conditions": "<code>loading={true}</code> and <code>loadingPosition=\"center\"</code>"
}
}
}
2 changes: 0 additions & 2 deletions packages/material-ui-lab/src/LoadingButton/LoadingButton.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,6 @@ export type LoadingButtonTypeMap<
endIconLoadingEnd?: string;
/** Styles applied to the startIcon element if `loading={true}` and `loadingPosition="start"`. */
startIconLoadingStart?: string;
/** Styles applied to the label element if `loading={true}` and `loadingPosition="center"`. */
labelLoadingCenter?: string;
};
/**
* If `true`, the loading indicator is shown.
Expand Down
24 changes: 13 additions & 11 deletions packages/material-ui-lab/src/LoadingButton/LoadingButton.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { chainPropTypes } from '@material-ui/utils';
import { capitalize } from '@material-ui/core/utils';
import { unstable_composeClasses as composeClasses } from '@material-ui/unstyled';
import { styled, unstable_useThemeProps as useThemeProps } from '@material-ui/core/styles';
import Button from '@material-ui/core/Button';
import Button, { buttonClasses } from '@material-ui/core/Button';
import CircularProgress from '@material-ui/core/CircularProgress';
import loadingButtonClasses, { getLoadingButtonUtilityClass } from './loadingButtonClasses';

Expand All @@ -15,7 +15,6 @@ const useUtilityClasses = (styleProps) => {
root: ['root', loading && 'loading'],
startIcon: [loading && `startIconLoading${capitalize(loadingPosition)}`],
endIcon: [loading && `endIconLoading${capitalize(loadingPosition)}`],
label: [loading && `labelLoading${capitalize(loadingPosition)}`],
loadingIndicator: [
'loadingIndicator',
loading && `loadingIndicator${capitalize(loadingPosition)}`,
Expand Down Expand Up @@ -51,22 +50,24 @@ const LoadingButtonRoot = styled(Button, {
...(styles.endIconLoadingEnd && {
[`& .${loadingButtonClasses.endIconLoadingEnd}`]: styles.endIconLoadingEnd,
}),
...(styles.labelLoadingCenter && {
[`& .${loadingButtonClasses.labelLoadingCenter}`]: styles.labelLoadingCenter,
}),
};
},
})({
})(({ styleProps, theme }) => ({
transition: theme.transitions.create(['background-color', 'box-shadow', 'border-color'], {
duration: theme.transitions.duration.short,
}),
Copy link
Member Author

Choose a reason for hiding this comment

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

add transition (exclude color) due to this glitch.

loading-button-transition.mp4

Copy link
Member

Choose a reason for hiding this comment

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

It still looks off:

looks off

The non centered cases are worse.

[`& .${loadingButtonClasses.startIconLoadingStart}`]: {
visibility: 'hidden',
},
[`& .${loadingButtonClasses.endIconLoadingEnd}`]: {
visibility: 'hidden',
},
[`& .${loadingButtonClasses.labelLoadingCenter}`]: {
visibility: 'hidden',
},
});
...(styleProps.loadingPosition === 'center' && {
[`&.${buttonClasses.disabled}`]: {
color: 'transparent',
},
}),
}));
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 to fix this.

image


const LoadingButtonLoadingIndicator = styled('div', {
name: 'MuiLoadingButton',
Expand All @@ -78,7 +79,7 @@ const LoadingButtonLoadingIndicator = styled('div', {
...styles[`loadingIndicator${capitalize(styleProps.loadingPosition)}`],
};
},
})(({ styleProps }) => ({
})(({ theme, styleProps }) => ({
position: 'absolute',
visibility: 'visible',
display: 'flex',
Expand All @@ -88,6 +89,7 @@ const LoadingButtonLoadingIndicator = styled('div', {
...(styleProps.loadingPosition === 'center' && {
left: '50%',
transform: 'translate(-50%)',
color: theme.palette.action.disabled,
Copy link
Member Author

@siriwatknp siriwatknp Jun 9, 2021

Choose a reason for hiding this comment

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

because of the above fix. color: transparent makes indicator disappear, so need to force the color same is disabled state in Button

}),
...(styleProps.loadingPosition === 'end' && {
right: 14,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ export interface LoadingButtonClasses {
endIconLoadingEnd: string;
/** Styles applied to the startIcon element if `loading={true}` and `loadingPosition="start"`. */
startIconLoadingStart: string;
/** Styles applied to the label element if `loading={true}` and `loadingPosition="center"`. */
labelLoadingCenter: string;
}

export type LoadingButtonClassKey = keyof LoadingButtonClasses;
Expand All @@ -36,7 +34,6 @@ const loadingButtonClasses: LoadingButtonClasses = generateUtilityClasses('MuiLo
'loadingIndicatorEnd',
'endIconLoadingEnd',
'startIconLoadingStart',
'labelLoadingCenter',
]);

export default loadingButtonClasses;
25 changes: 3 additions & 22 deletions packages/material-ui/src/Button/Button.js
Original file line number Diff line number Diff line change
Expand Up @@ -241,17 +241,6 @@ const ButtonRoot = styled(ButtonBase, {
},
);

const ButtonLabel = styled('span', {
name: 'MuiButton',
slot: 'Label',
overridesResolver: (props, styles) => styles.label,
})({
width: '100%', // Ensure the correct width for iOS Safari
display: 'inherit',
alignItems: 'inherit',
justifyContent: 'inherit',
});

const ButtonStartIcon = styled('span', {
name: 'MuiButton',
slot: 'StartIcon',
Expand Down Expand Up @@ -352,17 +341,9 @@ const Button = React.forwardRef(function Button(inProps, ref) {
{...other}
classes={classes}
>
{/*
* The inner <span> is required to vertically align the children.
* Browsers don't support `display: flex` on a <button> element.
* https://github.com/philipwalton/flexbugs/blob/master/README.md#flexbug-9
* TODO v5: evaluate if still required for the supported browsers.
*/}
<ButtonLabel className={classes.label} styleProps={styleProps}>
{startIcon}
{children}
{endIcon}
</ButtonLabel>
{startIcon}
{children}
{endIcon}
</ButtonRoot>
);
});
Expand Down
14 changes: 6 additions & 8 deletions packages/material-ui/src/Button/Button.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ describe('<Button />', () => {
const render = createClientRender();
const mount = createMount();

describeConformanceV5(<Button>Conformance?</Button>, () => ({
describeConformanceV5(<Button startIcon="icon">Conformance?</Button>, () => ({
classes,
inheritComponent: ButtonBase,
render,
mount,
refInstanceof: window.HTMLButtonElement,
muiName: 'MuiButton',
testDeepOverrides: { slotName: 'label', slotClassName: classes.label },
testDeepOverrides: { slotName: 'startIcon', slotClassName: classes.startIcon },
testVariantProps: { variant: 'contained', fullWidth: true },
testStateOverrides: { prop: 'size', value: 'small', styleKey: 'sizeSmall' },
skip: ['componentsProp'],
Expand Down Expand Up @@ -272,23 +272,21 @@ describe('<Button />', () => {
it('should render a button with startIcon', () => {
const { getByRole } = render(<Button startIcon={<span>icon</span>}>Hello World</Button>);
const button = getByRole('button');
const label = button.querySelector(`.${classes.label}`);
const startIcon = button.querySelector(`.${classes.startIcon}`);

expect(button).to.have.class(classes.root);
expect(button).to.have.class(classes.text);
expect(label.firstChild).not.to.have.class(classes.endIcon);
expect(label.firstChild).to.have.class(classes.startIcon);
expect(startIcon).not.to.have.class(classes.endIcon);
});

it('should render a button with endIcon', () => {
const { getByRole } = render(<Button endIcon={<span>icon</span>}>Hello World</Button>);
const button = getByRole('button');
const label = button.querySelector(`.${classes.label}`);
const endIcon = button.querySelector(`.${classes.endIcon}`);

expect(button).to.have.class(classes.root);
expect(button).to.have.class(classes.text);
expect(label.lastChild).not.to.have.class(classes.startIcon);
expect(label.lastChild).to.have.class(classes.endIcon);
expect(endIcon.lastChild).not.to.have.class(classes.startIcon);
siriwatknp marked this conversation as resolved.
Show resolved Hide resolved
});

it('should have a ripple by default', () => {
Expand Down
3 changes: 0 additions & 3 deletions packages/material-ui/src/Button/buttonClasses.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ import { generateUtilityClass, generateUtilityClasses } from '@material-ui/unsty
export interface ButtonClasses {
/** Styles applied to the root element. */
root: string;
/** Styles applied to the span element that wraps the children. */
label: string;
/** Styles applied to the root element if `variant="text"`. */
text: string;
/** Styles applied to the root element if `variant="text"` and `color="inherit"`. */
Expand Down Expand Up @@ -83,7 +81,6 @@ export function getButtonUtilityClass(slot: string): string {

const buttonClasses: ButtonClasses = generateUtilityClasses('MuiButton', [
'root',
'label',
'text',
'textInherit',
'textPrimary',
Expand Down