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

[Grid] Fix horizontal scrollbar and nested dimensions #24332

Merged
merged 12 commits into from Jan 15, 2021

Conversation

greguintow
Copy link
Contributor

@greguintow greguintow commented Jan 9, 2021

Fixes #7466

This PR is to be a way to implement what #24299 does without creating a new prop.

@mui-pr-bot
Copy link

mui-pr-bot commented Jan 9, 2021

Details of bundle changes

Generated by 🚫 dangerJS against 10a83b8

@oliviertassinari oliviertassinari changed the title Grid spacing [Grid] Fix horizontal scrollbar and nested dimensions Jan 10, 2021
@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work component: Grid The React component. labels Jan 10, 2021
@oliviertassinari oliviertassinari force-pushed the grid-spacing branch 2 times, most recently from 8a51140 to 588e4e7 Compare January 10, 2021 18:17
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.

I have polished it, looks good otherwise

@github-actions github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jan 11, 2021
@oliviertassinari oliviertassinari changed the title [Grid] Fix horizontal scrollbar and nested dimensions [Grid] Fix horizontal scrollbar Jan 12, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 12, 2021

@greguintow I have reverted the width nested handline. It's not something we can fix in the current configuration. Notice the explosion of the CSS generated:

HEAD
Capture d’écran 2021-01-12 à 13 30 36
Capture d’écran 2021-01-12 à 13 31 20

PR

Capture d’écran 2021-01-12 à 13 31 42

Capture d’écran 2021-01-12 à 13 31 01

One step at a time. We first need to migrate the Grid to emotion, then we should be able to apply the logic back and remove the !important by increasing specificity. This solves the concern of @eps1lon as we do no longer need the !important style.

@greguintow
Copy link
Contributor Author

@greguintow I have reverted the width nested handline. It's not something we can fix in the current configuration. Notice the explosion of the CSS generated:

It's showing 714 grid classes found but you have to consider this part of the code.

return {
  ...obj,
  [`&$container$item$spacing-xs-${spacing}`]: {
    flexBasis: fullWidth,
    maxWidth: fullWidth,
  },
};

This part shows that each breakpoint size will have this, but in that line have 3 .Mui-Grid, because will be .MuiGrid-container.MuiGrid-item.MuiGrid-spacing-xs-y. So the real amount of Mui-Grid would be 238 if we divide it by 3.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jan 12, 2021

@greguintow True, the only issue is that I already account for this in the search (notice all the whitespace prefixing the query). I would have found 2.5k matches if I didn't (x3) 😁

@greguintow
Copy link
Contributor Author

@greguintow True, the only issue is that I already account for this in the search (notice all the whitespace prefixing the query). I would have found 2.5k matches if I didn't (x3) 😁

LOOL, okay

@oliviertassinari
Copy link
Member

@greguintow But it's OK. We can work on the migration to emotion then applying the same solution. The size of the generated CSS should no longer be a concern.

@greguintow
Copy link
Contributor Author

@greguintow But it's OK. We can work on the migration to emotion then applying the same solution. The size of the generated CSS should no longer be a concern.

@oliviertassinari Alright sounds good! Does the migration to emotion change the component style in some way? Because I'm having to use version 5.0.0-alpha.20, because the Button migration made my app change completely, I think I should create an issue about it.

@oliviertassinari
Copy link
Member

Alright sounds good! Does the migration to emotion change the component style in some way? Because I'm having to use version 5.0.0-alpha.20, because the Button migration made my app change completely, I think I should create an issue about it.

You can find 10 duplicate issues about this in the issue tracker. There is a documented workaround only necessary during the phase were we have both jss and the new styled API.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2021
@oliviertassinari
Copy link
Member

I'm rebasing on top of #24395

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jan 15, 2021
@oliviertassinari oliviertassinari changed the title [Grid] Fix horizontal scrollbar [Grid] Fix horizontal scrollbar and nested dimensions Jan 15, 2021
@oliviertassinari oliviertassinari merged commit 1c68cfe into mui:next Jan 15, 2021
@oliviertassinari
Copy link
Member

@greguintow Thanks for sharing the solution and spending time on it :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work component: Grid The React component.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Grid] Extra width of container & scrollbar
4 participants