[Grid] Unexpected spacing behavior

[vicasas]

• The issue is present in the latest release.
• I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

In the Material UI documentation on the Grid component examples, it works fine inside the main container when using spacing

Expected Behavior 🤔

What is expected is that if I copy and paste the same example code to be able to test it will behave in the same way, but this does not work Link to codesandbox

It ends up generating a horizontal scroll.

Some solutions are given, but I don't see these being used in the example.

• We are using a Container component as parent

Steps to Reproduce 🕹

Steps:

1. Copy NestedGrid code example
2. Paste to codesandbox
3. Run app

Context 🔦

We were building a Layout for a page and we were in doubt regarding the spacing of the Grid component since we began to have several problems with horizontal scrolling.

Our implementation is

container: {
  paddingTop: 8,
  paddingBottom: 8,
}

<Container className={classes.container}>
  <Grid container spacing={2}>
    <Grid item xs>
      ...
    </Grid>
  </Grid>
</Container>

Your Environment 🌎

Material UI v4.11.2
React v17.0.1

`npx @material-ui/envinfo`

  Don't forget to mention which browser you used.
  Output from `npx @material-ui/envinfo` goes here.

[povilass]

I think it works as expected codeSandBox

Because spacing is calculated for each item in the respected container.

Or your example also works as expected.

[vicasas]

@povilass But if you enter the link of the sanbox that leaves, you will see that it does not work as expected in the example. Update the link because I was wrong

[oliviertassinari]

@vicasas Do you have more details on how it's wrong?

[vicasas]

@oliviertassinari Actually, I don't know if it is an error, but I don't understand why if I copy an example from Grid it doesn't work as in the documentation as it generates horizontal scrolling as shown in the codesanbox link.

Given this, I wonder if I am using Grid with correct spacing or not, since in my design it breaks generating horizontal scroll, so I should use any of the solutions indicated in the same documentation,

[oliviertassinari]

I can't reproduce the horizontal scrollbar you are referring to. What browser are you using? Which codesandbox?

[povilass]

Ok example showed problem bus still you need applied half spacing size to parent https://codesandbox.io/s/new-cache-lrynu like the documentation says.

[povilass]

As for documentation example fix should be added to root div sandbox.

Before

After

[vicasas]

I can't reproduce the horizontal scrollbar you are referring to. What browser are you using? Which codesandbox?

@oliviertassinari I am using Chrome in its version 87.0.4280.88 (Official Build) (64 bits), Material UI v4.11.2 and React v17.0.1.

To reproduce the "error" what I did was copy and paste the sample code of Basic Grid in a codesanbox, which automatically generates the horizontal scroll.

Example https://codesandbox.io/s/frosty-https-j4fzo?file=/src/App.js

Another way to reproduce the error is to run the example directly from the documentation by clicking on the codesandbox icon, when it opens it already comes with horizontal scroll

@povilass I think the same in the documentation examples should come spacing compensation for the example to work as expected. Or maybe add a more explicit example using Container as parent.

[oliviertassinari]

@vicasas Ok great, thanks. This is exactly why we have documented the limitation of the approach as well as workarounds. At this point, it's probably still too early to use flex-gap as not supported by iOS/Safari. But maybe #9513 will be our salvation.

Actually, this diff seems to fix the scrolling issue, could you confirm?

diff --git a/packages/material-ui/src/Grid/Grid.js b/packages/material-ui/src/Grid/Grid.js
index fa64775cfc..259596b6e3 100644
--- a/packages/material-ui/src/Grid/Grid.js
+++ b/packages/material-ui/src/Grid/Grid.js
@@ -65,9 +65,9 @@ function generateGrid(globalStyles, theme, breakpoint) {
   }
 }

-function getOffset(val, div = 1) {
+function getOffset(val) {
   const parse = parseFloat(val);
-  return `${parse / div}${String(val).replace(String(parse), '') || 'px'}`;
+  return `${parse / 2}${String(val).replace(String(parse), '') || 'px'}`;
 }

 function generateGutter(theme, breakpoint) {
@@ -81,10 +81,12 @@ function generateGutter(theme, breakpoint) {
     }

     styles[`spacing-${breakpoint}-${spacing}`] = {
-      margin: `-${getOffset(themeSpacing, 2)}`,
+      marginTop: `-${getOffset(themeSpacing)}`,
+      marginLeft: `-${getOffset(themeSpacing)}`,
       width: `calc(100% + ${getOffset(themeSpacing)})`,
       '& > $item': {
-        padding: getOffset(themeSpacing, 2),
+        paddingTop: getOffset(themeSpacing),
+        paddingLeft: getOffset(themeSpacing),
       },
     };
   });

I can't catch regressions. Proof of concept: https://codesandbox.io/s/keen-feather-rdgty.

[vicasas]

@oliviertassinari I tried with that change but the horizontal scrolling still appears. I was comparing how Bootstrap for Grid works, but didn't see more.

In the same way, I think it is fine, but the Grid examples of Material UI should be added compensation in the root as indicated in it.

For example Basic grid

root:  {
  padding: 8
}

<div className={classes.root}>
  <Grid container spacing={3}>
        ...
  </Grid>
</div>

ór add this example

container: {
  paddingTop: 8,
  paddingBottom: 8,
}

<Container className={classes.container}>
  <Grid container spacing={2}>
    <Grid item xs>
      ...
    </Grid>
  </Grid>
</Container>

[povilass]

But it works I applied changes to Grid component and called it Grid2 sandbox.

Example 1 with scroll

New grid example 1 no scroll

Important When you change property leave just one of them and press codesandbox refresh button.

I reproduced issues in Windows 10 Chrome Version 87.0.4280.88 (Official Build) (64-bit) and Microsoft Edge Version 87.0.664.66 (Official build) (64-bit).

Maybe it's working if you are using MAC+Chrome. MAC+Chrome not equal to WINDOWS+Chrome.

[oliviertassinari]

https://codesandbox.io/s/eloquent-ganguly-oxfzk?file=/src/App.js

@povilass Thanks for the codesandbox. I have tested it in macOS Chome and Windows Chrome latest version, no scrollbar.

[povilass]

@oliviertassinari Ok this is preview.

[oliviertassinari]

@povilass Ok, so we can reproduce the problem as well as reproduce the solution is working. It sounds like we can move forward with a pull request.

[vicasas]

@oliviertassinari I doubt, is it correct to change the spacing of the 4 axes and leave it to the left and up? In grid layouts, the columns (element) always have their padding on their 4 axes.

Personally, I would settle for adding in the Grid examples that overflow padding at the root.

[greguintow]

Hey guys, I fixed that like 2 weeks ago. But I didn't get time to do the PR, may I send the PR and just tag this issue without giving a description?

[vicasas]

@greguintow Can we see the solution? in a codesandbox

[greguintow]

@vicasas Okay, I'll prepare one

[greguintow]

@vicasas Here you go Demo. My solution works through a fullWidth prop

[oliviertassinari]

@greguintow Why do we need to add an extra prop? What's wrong with the existing proposed solution?

[greguintow]

@oliviertassinari I've tried to reproduce the same example I used on mine in the proposed solution and didn't work as it should.

[oliviertassinari]

@greguintow Do you have a reproduction?

[greguintow]

@greguintow Do you have a reproduction?

I changed on the code sandbox if you want I can add to my demo to compare it. Answering your question about why I decided to go with a prop, it's because I didn't want to cause any breaking change.

[oliviertassinari]

What breaking change are you referring to? Unless #24272 (comment) has a flaw we aren't aware of, we should continue forward with the approach. Adding a new prop isn't OK as a solution.

[vicasas]

@vicasas Aquí tienes Demo . Mi solución funciona a través de un accesorio de ancho completo

@greguintow I keep seeing the same scroll problem

[greguintow]

@vicasas Wow, I really haven't noticed that, I was wondering why there was a blank space on my app haha, okay let me try to fix that.

[oliviertassinari]

@vicasas Could you try https://codesandbox.io/s/keen-feather-rdgty?

[greguintow]

@greguintow Do you have a reproduction?

This is I using my example in the proposed solution https://codesandbox.io/s/awesome-dream-21ynb

[vicasas]

@vicasas ¿Podrías probar https://codesandbox.io/s/keen-feather-rdgty ?

@oliviertassinari That approach works, image attached.

But I wonder if item should only have left and top padding instead of having them on its 4 axes. The question comes to the fact that other Grids always have padding in the 4 axes🤔

[oliviertassinari]

But I wonder if item should only have left and top padding instead of having them on its 4 axes.

I don't think that it matter

[oliviertassinari]

This is I using my example in the proposed solution Demo

@greguintow It looks like this issue is already existing and different from the scrollbar.

[greguintow]

@oliviertassinari Got it. So there is a serious bug with the proposed solution regarding the spacing.
https://codesandbox.io/s/angry-kapitsa-wgqek

Ignore what you guys saw before on the old section, it was using the latest instead of the next version.
You can see normally now:
https://codesandbox.io/s/angry-kapitsa-wgqek

[greguintow]

@oliviertassinari Alright, after a lot of headaches, I could figure out the solution and not just fixing the scrollbar problem, but also the spacing problem.
Please check it out: https://codesandbox.io/s/angry-kapitsa-wgqek

[greguintow]

@oliviertassinari Let me know if you want to me remove the fullWidth prop and make native support. But please reflect that would be a breaking change, as many users use Grid in a lot of parts of the website and they may have their own fixes, so might be a problem. But native support would probably be better for the bundle as the prop adds around 800 classes because create a class for each spacing for each breakpoint size in each breakpoint.

[greguintow]

@oliviertassinari On PR #24332 there is the same solution without the fullWidth prop that was able to reduce 4x times the ungzipped size of the Grid component comparing to my previous solution.

[oliviertassinari]

Duplicate of #7466