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

[Container] Fix mismatch between Container and Toolbar gutters #19505

Merged
merged 2 commits into from
Feb 2, 2020

Conversation

koistya
Copy link
Contributor

@koistya koistya commented Feb 1, 2020

Fixes a mismatch between Container and Toolbar gutters when the screen size is larger than md.

@koistya koistya changed the title Fix gutters in Toolbar to match Container Fixe a mismatch between Container and Toolbar gutters Feb 1, 2020
@koistya koistya changed the title Fixe a mismatch between Container and Toolbar gutters Fix a mismatch between Container and Toolbar gutters Feb 1, 2020
@mui-pr-bot
Copy link

mui-pr-bot commented Feb 1, 2020

Details of bundle changes.

Comparing: 39c8170...15a7c13

bundle Size Change Size Gzip Change Gzip
@material-ui/core ▼ -105 B (-0.03% ) 361 kB ▼ -32 B (-0.03% ) 98.7 kB
docs.main ▼ -105 B (-0.02% ) 596 kB ▼ -30 B (-0.02% ) 194 kB
Container ▼ -105 B (-0.17% ) 63.3 kB ▼ -21 B (-0.11% ) 19.8 kB
@material-ui/core[umd] ▼ -95 B (-0.03% ) 317 kB ▼ -28 B (-0.03% ) 91.9 kB
@material-ui/lab -- 185 kB -- 55.3 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
@material-ui/system -- 14.8 kB -- 4.07 kB
Alert -- 84 kB -- 26.3 kB
AlertTitle -- 64.3 kB -- 20.3 kB
AppBar -- 64.1 kB -- 20.1 kB
Autocomplete -- 132 kB -- 41.4 kB
Avatar -- 65.4 kB -- 20.7 kB
AvatarGroup -- 62.4 kB -- 19.6 kB
Backdrop -- 68 kB -- 21 kB
Badge -- 65.5 kB -- 20.4 kB
BottomNavigation -- 62.6 kB -- 19.6 kB
BottomNavigationAction -- 75.7 kB -- 23.9 kB
Box -- 71 kB -- 21.7 kB
Breadcrumbs -- 67.9 kB -- 21.3 kB
Button -- 79.9 kB -- 24.5 kB
ButtonBase -- 74.2 kB -- 23.3 kB
ButtonGroup -- 83.4 kB -- 25.5 kB
Card -- 63 kB -- 19.7 kB
CardActionArea -- 75.3 kB -- 23.7 kB
CardActions -- 62.2 kB -- 19.5 kB
CardContent -- 62.1 kB -- 19.5 kB
CardHeader -- 65.2 kB -- 20.5 kB
CardMedia -- 62.5 kB -- 19.7 kB
Checkbox -- 83.2 kB -- 26.3 kB
Chip -- 82.8 kB -- 25.4 kB
CircularProgress -- 64.3 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.2 kB -- 21.1 kB
colorManipulator -- 3.88 kB -- 1.52 kB
CssBaseline -- 57.7 kB -- 18.1 kB
Dialog -- 83.2 kB -- 25.9 kB
DialogActions -- 62.2 kB -- 19.5 kB
DialogContent -- 62.4 kB -- 19.6 kB
DialogContentText -- 64.2 kB -- 20.2 kB
DialogTitle -- 64.4 kB -- 20.2 kB
Divider -- 62.7 kB -- 19.7 kB
docs.landing -- 60.5 kB -- 16.4 kB
Drawer -- 85 kB -- 25.8 kB
ExpansionPanel -- 72.5 kB -- 22.7 kB
ExpansionPanelActions -- 62.2 kB -- 19.5 kB
ExpansionPanelDetails -- 62.1 kB -- 19.5 kB
ExpansionPanelSummary -- 78.3 kB -- 24.7 kB
Fab -- 77 kB -- 24 kB
Fade -- 23.4 kB -- 7.98 kB
FilledInput -- 73.7 kB -- 22.9 kB
FormControl -- 64.6 kB -- 20.2 kB
FormControlLabel -- 65.7 kB -- 20.7 kB
FormGroup -- 62.2 kB -- 19.5 kB
FormHelperText -- 63.5 kB -- 20 kB
FormLabel -- 63.6 kB -- 19.8 kB
Grid -- 65.3 kB -- 20.5 kB
GridList -- 62.6 kB -- 19.7 kB
GridListTile -- 63.9 kB -- 20 kB
GridListTileBar -- 63.4 kB -- 19.9 kB
Grow -- 24 kB -- 8.19 kB
Hidden -- 66.1 kB -- 20.8 kB
Icon -- 62.9 kB -- 19.8 kB
IconButton -- 76.3 kB -- 23.8 kB
Input -- 72.7 kB -- 22.7 kB
InputAdornment -- 65.3 kB -- 20.5 kB
InputBase -- 70.8 kB -- 22.2 kB
InputLabel -- 65.5 kB -- 20.2 kB
LinearProgress -- 65.5 kB -- 20.5 kB
Link -- 66.8 kB -- 21.1 kB
List -- 62.5 kB -- 19.5 kB
ListItem -- 77.3 kB -- 24.2 kB
ListItemAvatar -- 62.3 kB -- 19.5 kB
ListItemIcon -- 62.3 kB -- 19.5 kB
ListItemSecondaryAction -- 62.2 kB -- 19.5 kB
ListItemText -- 65.1 kB -- 20.5 kB
ListSubheader -- 62.9 kB -- 19.8 kB
Menu -- 89 kB -- 27.4 kB
MenuItem -- 78.4 kB -- 24.5 kB
MenuList -- 66.2 kB -- 20.7 kB
MobileStepper -- 68 kB -- 21.4 kB
Modal -- 14.5 kB -- 5.05 kB
NativeSelect -- 77 kB -- 24.3 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 74.7 kB -- 23.3 kB
Paper -- 62.5 kB -- 19.5 kB
Popover -- 83.3 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
Radio -- 84.2 kB -- 26.5 kB
RadioGroup -- 64.6 kB -- 20.1 kB
Rating -- 70.7 kB -- 22.7 kB
RootRef -- 4.24 kB -- 1.64 kB
Select -- 117 kB -- 34.6 kB
Skeleton -- 63.1 kB -- 20 kB
Slide -- 25.5 kB -- 8.71 kB
Slider -- 76.8 kB -- 24.3 kB
Snackbar -- 75.6 kB -- 23.7 kB
SnackbarContent -- 63.7 kB -- 20.1 kB
SpeedDial -- 86.4 kB -- 27.2 kB
SpeedDialAction -- 119 kB -- 37.5 kB
SpeedDialIcon -- 64.7 kB -- 20.3 kB
Step -- 62.8 kB -- 19.7 kB
StepButton -- 82.5 kB -- 26.1 kB
StepConnector -- 62.9 kB -- 19.8 kB
StepContent -- 69.3 kB -- 21.7 kB
StepIcon -- 64.8 kB -- 20.2 kB
StepLabel -- 68.8 kB -- 21.7 kB
Stepper -- 65 kB -- 20.5 kB
styles/createMuiTheme -- 16.5 kB -- 5.81 kB
SvgIcon -- 63.2 kB -- 19.8 kB
SwipeableDrawer -- 92.4 kB -- 28.9 kB
Switch -- 82.3 kB -- 26 kB
Tab -- 76.5 kB -- 24.2 kB
Table -- 62.7 kB -- 19.7 kB
TableBody -- 62.3 kB -- 19.5 kB
TableCell -- 64.2 kB -- 20.2 kB
TableContainer -- 62.1 kB -- 19.5 kB
TableFooter -- 62.3 kB -- 19.5 kB
TableHead -- 62.3 kB -- 19.5 kB
TablePagination -- 144 kB -- 42 kB
TableRow -- 62.6 kB -- 19.7 kB
TableSortLabel -- 77.6 kB -- 24.4 kB
Tabs -- 85.8 kB -- 27.2 kB
TextareaAutosize -- 5.12 kB -- 2.14 kB
TextField -- 125 kB -- 36.6 kB
ToggleButton -- 76.3 kB -- 24.2 kB
ToggleButtonGroup -- 63.4 kB -- 20 kB
Toolbar -- 62.5 kB -- 19.6 kB
Tooltip -- 102 kB -- 32.4 kB
TreeItem -- 74.2 kB -- 23.5 kB
TreeView -- 66.8 kB -- 21.1 kB
Typography -- 63.8 kB -- 20 kB
useAutocomplete -- 14.6 kB -- 5.3 kB
useMediaQuery -- 2.58 kB -- 1.07 kB
Zoom -- 23.5 kB -- 8.1 kB

Generated by 🚫 dangerJS against 15a7c13

@oliviertassinari
Copy link
Member

Why should it match?

@oliviertassinari oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Feb 1, 2020
@koistya
Copy link
Contributor Author

koistya commented Feb 1, 2020

Why should it match?

I don't know, maybe it shouldn't, but it looks weird. Without it, the content area inside of the Toolbar is wider than the content area inside of the container. Note, that this mismatch only takes place on screens larger than md (on xs and sm screen sizes, the content inside of the Toolbar and Container is perfectly aligned).

Here is an example (AppBar > Toolbar at the top, then goes Container right below the AppBar):

Before

image

After

image

@oliviertassinari
Copy link
Member

What do you think of reducing the container padding instead?

diff --git a/packages/material-ui/src/Container/Container.js b/packages/material-ui/src/Container/Container.js
index ea24a213b..82716c59c 100644
--- a/packages/material-ui/src/Container/Container.js
+++ b/packages/material-ui/src/Container/Container.js
@@ -17,10 +17,6 @@ export const styles = theme => ({
       paddingLeft: theme.spacing(3),
       paddingRight: theme.spacing(3),
     },
-    [theme.breakpoints.up('md')]: {
-      paddingLeft: theme.spacing(4),
-      paddingRight: theme.spacing(4),
-    },
   },
   /* Styles applied to the root element if `disableGutters={true}`. */
   disableGutters: {

@koistya
Copy link
Contributor Author

koistya commented Feb 1, 2020

@oliviertassinari that works for me. Just updated the PR. Let's see how the argos screenshots would look like in this case.

@oliviertassinari oliviertassinari changed the title Fix a mismatch between Container and Toolbar gutters [Container] Fix mismatch between Container and Toolbar gutters Feb 1, 2020
@oliviertassinari oliviertassinari added component: Container The React component and removed PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Feb 1, 2020
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.

While I don't see a strong use case for making the Container and the Toolbar match, benchmarking (I have looked at Bootstrap, GitHub, Apple, Stripe, and Tailwind), it seems that simplifying the padding logic of the Container is a good direction.

@oliviertassinari
Copy link
Member

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: Container The React component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants