Skip to content

Conversation

@siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jun 24, 2021

BREAKING CHANGE


WHY the wrapper is removed
button > span > children exists as a workaround for flex container bug on button BUT not anymore, so this PR remove the span.

  • ✅ Chrome 83+ (latest 90)
  • ✅ Safari 11+ (latest 14)
  • ✅ Edge
  • ✅ Firefox 63 (latest 89)

https://github.com/philipwalton/flexbugs#flexbug-9


Preview: https://deploy-preview-26926--material-ui.netlify.app/components/tabs/#icon-tabs

@siriwatknp siriwatknp added the scope: tabs Changes related to the tabs. label Jun 24, 2021
@mui-pr-bot
Copy link

mui-pr-bot commented Jun 24, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 2602126

/* Styles applied to the root element if `wrapped={true}`. */
...(styleProps.wrapped && {
fontSize: theme.typography.pxToRem(12),
lineHeight: 1.5,
Copy link
Member Author

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

@siriwatknp siriwatknp added the breaking change Introduces changes that are not backward compatible. label Jun 24, 2021
@oliviertassinari oliviertassinari changed the title [Tabs] remove wrapper from Tab [Tabs] Remove unnecessary wrapper from Tab Jun 24, 2021
Comment on lines 1588 to 1590
- 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

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

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

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

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

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

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.

@siriwatknp siriwatknp requested a review from mnajdova June 25, 2021 03:21
@mnajdova
Copy link
Member

Overall, the changes look good to me 👍

@siriwatknp siriwatknp merged commit 0534ae0 into mui:next Jun 28, 2021
@siriwatknp siriwatknp deleted the fix/remove-tab-wrapper branch June 28, 2021 03:25
@jmgomezdev
Copy link

In the realease v5.0.0-beta, you have removed no longer necessary span wrapper in tab buttons. The problem is that the missing span with the MuiTab-wrapper class results in insufficient spacing.

v5.0.0-beta.1 => https://codesandbox.io/s/tab-buttons-v5-nv6c1
v4.12.2 => https://codesandbox.io/s/n5uch

@oliviertassinari
Copy link
Member

@jmgomezdev This change doesn't lead to any visual change. Aren't you looking for #26458 instead? In any case, it's expected.

@jmgomezdev
Copy link

Yes, you're right

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change Introduces changes that are not backward compatible. scope: tabs Changes related to the tabs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants