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

[Table] Adjust table styles to the latest specs #17388

Merged
merged 3 commits into from
Sep 22, 2019

Conversation

kybarg
Copy link
Contributor

@kybarg kybarg commented Sep 11, 2019

On July 25, 2019 Google updated Specs on Table component. Padding and fonts were updated

@mui-pr-bot
Copy link

mui-pr-bot commented Sep 11, 2019

Details of bundle changes.

Comparing: 1a480fe...82753b3

bundle parsed diff gzip diff prev parsed current parsed prev gzip current gzip
@material-ui/core -0.06% -0.04% 331,688 331,498 90,532 90,495
@material-ui/core/Paper 0.00% 0.00% 68,659 68,659 20,458 20,458
@material-ui/core/Paper.esm 0.00% 0.00% 62,098 62,098 19,203 19,203
@material-ui/core/Popper 0.00% 0.00% 28,397 28,397 10,159 10,159
@material-ui/core/Textarea 0.00% 0.00% 5,082 5,082 2,132 2,132
@material-ui/core/TrapFocus 0.00% 0.00% 3,766 3,766 1,596 1,596
@material-ui/core/styles/createMuiTheme 0.00% 0.00% 16,348 16,348 5,818 5,818
@material-ui/core/useMediaQuery 0.00% 0.00% 2,488 2,488 1,050 1,050
@material-ui/lab 0.00% 0.00% 154,749 154,749 46,885 46,885
@material-ui/styles 0.00% 0.00% 51,420 51,420 15,288 15,288
@material-ui/system 0.00% 0.00% 15,581 15,581 4,351 4,351
Button 0.00% 0.00% 78,610 78,610 24,024 24,024
Modal 0.00% 0.00% 14,542 14,542 5,114 5,114
Portal 0.00% 0.00% 2,907 2,907 1,318 1,318
Rating 0.00% 0.00% 69,963 69,963 21,856 21,856
Slider 0.00% 0.00% 75,084 75,084 23,184 23,184
colorManipulator 0.00% 0.00% 3,835 3,835 1,519 1,519
docs.landing 0.00% 0.00% 52,232 52,232 13,768 13,768
docs.main -0.04% -0.05% 597,197 596,953 190,673 190,586
packages/material-ui/build/umd/material-ui.production.min.js -0.06% -0.04% 302,599 302,409 86,969 86,937

Generated by 🚫 dangerJS against 82753b3

@mbrookes

This comment has been minimized.

@kybarg

This comment has been minimized.

@mbrookes

This comment has been minimized.

@oliviertassinari oliviertassinari added this to the v4.5.0 milestone Sep 12, 2019
@oliviertassinari oliviertassinari added component: table This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process labels Sep 12, 2019
@oliviertassinari
Copy link
Member

I'm on my phone, I can't have deep dive yet. I think that we should update the max height value of the "fixed header" demo to account the change of the rows' height.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

The question about the width of the cell was, why do the table cells take up more horizontal space than they need to? (Not specific to this PR, just in general.)

@mbrookes The table layout mode has always puzzled me 😁. In this case, it's on purpose, the demo tries to showcase vertical and horizontal scrolling support.

@mbrookes
Copy link
Member

Oh, we have min-width set. I don't know how I missed it!! 🤦‍♂️

@kybarg
Copy link
Contributor Author

kybarg commented Sep 18, 2019

@mbrookes @oliviertassinari Speaking about outer border...
In order to make outer border we need to wrap Table content into some other element, and I'm note sure if it is a big change. I think most of users (like myself) already have their tables wrapped into Paper component.
Is not it better move to make changes to Paper to render outer border at elevation=0 or introduce outlined prop?

@oliviertassinari
Copy link
Member

@kybarg Interesting, I don't think that the table component should come with an outline by default. However, I think that would be great to support it. I think that we could have two follow-up pull requests:

What do you think?

@oliviertassinari oliviertassinari merged commit 6a1c305 into mui:master Sep 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: table This is the name of the generic UI component, not the React module! design: material This is about Material Design, please involve a visual or UX designer in the process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants