Skip to content

[Tabs] Remove unnecessary wrapper from Tab #26926

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.

Sign up for GitHub

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

Merged
siriwatknp merged 3 commits into from
Jun 28, 2021
Merged

[Tabs] Remove unnecessary wrapper from Tab #26926

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
59ee701
remove wrapper from Tab
siriwatknp Jun 24, 2021
8365b93
remove testDeepOverrides
siriwatknp Jun 24, 2021
2602126
update link
siriwatknp Jun 28, 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
3 changes: 1 addition & 2 deletions docs/pages/api-docs/tab.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,7 @@
"selected",
"disabled",
"fullWidth",
"wrapped",
"wrapper"
"wrapped"
],
"globalClasses": { "selected": "Mui-selected", "disabled": "Mui-disabled" },
"name": "MuiTab"
Expand Down
12 changes: 12 additions & 0 deletions docs/src/pages/guides/migration-v4/migration-v4.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1583,8 +1583,20 @@ You can use the [`collapse-rename-collapsedheight` codemod](https://github.com/m
+<Tabs scrollButtons={false} />
```

### Tab

- Tab `minWidth` changed from `72px` => `90px` (without media-query) according to [material-design spec](https://material.io/components/tabs#specs)
- Tab `maxWidth` changed from `264px` => `360px` according to [material-design spec](https://material.io/components/tabs#specs)
- `span` element that wraps children has been removed. `wrapper` classKey is also removed. More details about [this change](https://github.com/mui-org/material-ui/pull/26666).
Copy link
Member

@oliviertassinari oliviertassinari Jun 24, 2021

Choose a reason for hiding this comment

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

The passive voice doesn't feel right. Most of the items in this guide are in the active voice.

Suggested change
- Tab `minWidth` changed from `72px` => `90px` (without media-query) according to [material-design spec](https://material.io/components/tabs#specs)
- Tab `maxWidth` changed from `264px` => `360px` according to [material-design spec](https://material.io/components/tabs#specs)
- `span` element that wraps children has been removed. `wrapper` classKey is also removed. More details about [this change](https://github.com/mui-org/material-ui/pull/26666).
- Change the Tab `minWidth` from `72px` => `90px` (without media-query) according to [material-design spec](https://material.io/components/tabs#specs)
- Change the Tab `maxWidth` from `264px` => `360px` according to [material-design spec](https://material.io/components/tabs#specs)
- Remove the `span` element that wraps children. Remove the `wrapper` classKey too. More details about [this change](https://github.com/mui-org/material-ui/pull/26666).

cc @mbrookes for confirmation.

Copy link
Member

@mnajdova mnajdova Jun 25, 2021

Choose a reason for hiding this comment

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

I think it's confusing linking different PR here. Let's update the current PR description with the details of why the changes was introduced and link this PR in the migration guide.

Copy link
Member

@mbrookes mbrookes Jun 27, 2021

Choose a reason for hiding this comment

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

The passive voice doesn't feel right.

I'm not so sure. "Change the Tab minWidth" sounds like an instruction to the reader, whereas "Tab minWidth (was/has been) changed" describes what has changed:

  • What changed
  • What is/are the implication(s)
  • What you need to do

(Situation, Complication, Resolution...)

Copy link
Member

@eps1lon eps1lon Jun 27, 2021

Choose a reason for hiding this comment

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

Intuitively I would've also preferred passive voice in changelogs.

Though it's not uncommon for changelogs to have active voice:

changelogs with passive voice:

I could go on forever and find examples for active and passive voice. Though I can't find any resources on active vs passive voice for changelogs specifically (this is why "benchmarking" is generally useless to form an actionable advise).

I'd say it's more important to use a consistent voice throughout the document since I'm fine with either voice. Though definitely an interesting topic.

Copy link
Member

@oliviertassinari oliviertassinari Jun 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for the two examples. Would it work if we use this framing?

  1. In the changelog, the PR's description are in the active voice because they describe the action of the change. It's more git focused. (what we have done so far)
  2. In the changelog's summary, the passive voice is used, because we focus on the developers not familiar with the codebase reading it. (what we have done so far)
  3. In the migration guide, the passive voice is used for the same reason as 3. (currently, a mix is used)

Copy link
Member Author

@siriwatknp siriwatknp Jun 28, 2021

Choose a reason for hiding this comment

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

similar thought as Sebastian, I am okay with the above proposal as long as we keep them consistent.


```diff
<button class="MuiTab-root">
- <span class="MuiTab-wrapper">
{icon}
{label}
- </span>
</button>
```

### TextField

Expand Down
4 changes: 0 additions & 4 deletions docs/translations/api-docs/tab/tab.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,6 @@
"description": "Styles applied to {{nodeName}} if {{conditions}}.",
"nodeName": "the root element",
"conditions": "<code>wrapped={true}</code>"
},
"wrapper": {
"description": "Styles applied to {{nodeName}}.",
"nodeName": "the `icon` and `label`'s wrapper element"
}
}
}
26 changes: 5 additions & 21 deletions packages/material-ui/src/Tab/Tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@ const useUtilityClasses = (styleProps) => {
selected && 'selected',
disabled && 'disabled',
],
wrapper: ['wrapper'],
};

return composeClasses(slots, getTabUtilityClass, classes);
Expand Down Expand Up @@ -54,13 +53,15 @@ const TabRoot = styled(ButtonBase, {
overflow: 'hidden',
whiteSpace: 'normal',
textAlign: 'center',
flexDirection: 'column',
lineHeight: 1.25,
/* Styles applied to the root element if both `icon` and `label` are provided. */
...(styleProps.icon &&
styleProps.label && {
minHeight: 72,
paddingTop: 9,
paddingBottom: 9,
[`& .${tabClasses.wrapper} > *:first-child`]: {
[`& > *:first-child`]: {
marginBottom: 6,
},
}),
Expand Down Expand Up @@ -105,24 +106,9 @@ const TabRoot = styled(ButtonBase, {
/* Styles applied to the root element if `wrapped={true}`. */
...(styleProps.wrapped && {
fontSize: theme.typography.pxToRem(12),
lineHeight: 1.5,
Copy link
Member Author

@siriwatknp siriwatknp Jun 24, 2021

Choose a reason for hiding this comment

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

this can be removed because TabWrapper also has lineHeight

}),
}));

const TabWrapper = styled('span', {
name: 'MuiTab',
slot: 'Wrapper',
overridesResolver: (props, styles) => styles.wrapper,
})({
/* Styles applied to the `icon` and `label`'s wrapper element. */
display: 'inline-flex',
alignItems: 'center',
justifyContent: 'center',
width: '100%',
flexDirection: 'column',
lineHeight: 1.25,
});

const Tab = React.forwardRef(function Tab(inProps, ref) {
const props = useThemeProps({ props: inProps, name: 'MuiTab' });
const {
Expand Down Expand Up @@ -197,10 +183,8 @@ const Tab = React.forwardRef(function Tab(inProps, ref) {
tabIndex={selected ? 0 : -1}
{...other}
>
<TabWrapper className={classes.wrapper} styleProps={styleProps}>
{icon}
{label}
</TabWrapper>
{icon}
{label}
{indicator}
</TabRoot>
);
Expand Down
3 changes: 0 additions & 3 deletions packages/material-ui/src/Tab/tabClasses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@ export interface TabClasses {
fullWidth: string;
/** Styles applied to the root element if `wrapped={true}`. */
wrapped: string;
/** Styles applied to the `icon` and `label`'s wrapper element. */
wrapper: string;
}

export type TabClassKey = keyof TabClasses;
Expand All @@ -39,7 +37,6 @@ const tabClasses: TabClasses = generateUtilityClasses('MuiTab', [
'disabled',
'fullWidth',
'wrapped',
'wrapper',
]);

export default tabClasses;