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

[Tabs] Simplify override #14638

Merged
merged 1 commit into from
Feb 24, 2019

Conversation

oliviertassinari
Copy link
Member

@oliviertassinari oliviertassinari commented Feb 23, 2019

Breaking change

We have removed the labelContainer, label and labelWrapped class keys. We have removed 2 intermediary DOM elements. You should be able to move the custom styles to the root class key.

capture d ecran 2019-02-23 a 15 46 48

@oliviertassinari oliviertassinari added breaking change component: tabs This is the name of the generic UI component, not the React module! labels Feb 23, 2019
@eps1lon eps1lon mentioned this pull request Feb 23, 2019
56 tasks
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 23, 2019

@material-ui/core: parsed: -0.18%:heart_eyes:, gzip: -0.11%:heart_eyes:

Details of bundle changes.

Comparing: cbed923...536ef7b

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core/Paper 0.00% 0.00% 83,040 83,040 21,410 21,410
@material-ui/core/Paper.esm 0.00% 0.00% 76,728 76,728 20,462 20,462
@material-ui/core -0.18% -0.11% 484,877 484,024 130,093 129,946
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,235 16,235 5,159 5,159
@material-ui/lab 0.00% 0.00% 295,473 295,473 86,174 86,174
@material-ui/styles 0.00% 0.00% 63,755 63,755 18,119 18,119
@material-ui/system 0.00% 0.00% 15,915 15,915 3,938 3,938
colorManipulator 0.00% 0.00% 2,290 2,290 838 838
Button 0.00% 0.00% 212,888 212,888 61,970 61,970
Modal 0.00% 0.00% 211,094 211,094 61,687 61,687
@material-ui/core/Popper 0.00% 0.00% 146,815 146,815 46,826 46,826
@material-ui/core/useMediaQuery 0.00% 0.00% 9,217 9,217 3,503 3,503
docs.main 0.00% 0.00% 676,139 676,139 205,127 205,127
docs.landing 0.00% 0.00% 48,963 48,963 10,275 10,275
packages/material-ui/build/umd/material-ui.production.min.js -0.24% -0.16% 320,511 319,754 84,731 84,598

Generated by 🚫 dangerJS against 536ef7b

@oliviertassinari oliviertassinari force-pushed the tab-move-padding-style branch 3 times, most recently from dec2076 to bfb0760 Compare February 23, 2019 15:51
marginRight: 30,
marginLeft: 30,
},
[attach(TAB.selected)]: {
Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @siriwatknp, If you have some time, I think that it would be great that we take the time to find the best possible theme implementation. We have different theme demos, the approach isn't consistent. I think that we should try to unify them.

Copy link
Member

Choose a reason for hiding this comment

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

Hi @oliviertassinari, should we open a discussion topic about mui-theming?

@mbrookes
Copy link
Member

mbrookes commented Feb 24, 2019

@material-ui/core: parsed: -0.18%😍, gzip: -0.11%😍

Better than 0.011% I guess, but prbot might be being a little over-enthusiastic here 😄

@oliviertassinari oliviertassinari merged commit ee703ab into mui:next Feb 24, 2019
@oliviertassinari oliviertassinari deleted the tab-move-padding-style branch February 24, 2019 12:16
[nest(ICON.root)]: {
minHeight: null,
paddingTop: null,
'& $wrapper :first-child': {
Copy link
Contributor

@HaNdTriX HaNdTriX Mar 10, 2019

Choose a reason for hiding this comment

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

Oh this rule applies to all first-childs in the tree. This messes things up, if you are using icons and labels.

'& $wrapper :first-child' === '& $wrapper *:first-child'

I think you want to omit the space here:

'& $wrapper:first-child'

Copy link
Contributor

@HaNdTriX HaNdTriX Mar 10, 2019

Choose a reason for hiding this comment

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

In my case I am using Tabs with Labels, Icons and Badges. The above rule is applied to all of them.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you using this custom theme? Yeah, I'm happy to try the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh actually not. I added this comment to the wrong part of the code 😬. Nevertheless the issue stays the same. You might want look at this, too.

https://github.com/mui-org/material-ui/blob/8a13bc294e04c1af8fd5cf69a287132974b81577/packages/material-ui/src/Tab/Tab.js#L37-L39

Copy link
Contributor

@HaNdTriX HaNdTriX Mar 10, 2019

Choose a reason for hiding this comment

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

Copy link
Member Author

@oliviertassinari oliviertassinari Mar 10, 2019

Choose a reason for hiding this comment

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

What about?

'& $wrapper > *:first-child': {

Copy link
Contributor

@HaNdTriX HaNdTriX Mar 10, 2019

Choose a reason for hiding this comment

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

testes locally & lgtm! 😍

Copy link
Member Author

Choose a reason for hiding this comment

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

Awesome! Do you want to submit a pull request? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to!

Copy link
Contributor

Choose a reason for hiding this comment

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

Done #14844

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change component: tabs This is the name of the generic UI component, not the React module!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants