[Grid] Better document direction column limitation

[cyrfer]

Grid items are an opinionated height when using the xs and similar attributes on items within Grid column containers.

I don't think you should use flex-basis for Grid item's in a column container when xs and similar attributes are used on items. I think xs and related attributes are intended to control the width only, never the height, but flex-basis controls height in column containers.

• 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 😯

The following code results in an item with flex-basis: 100%, which is not expected for column containers.

<Grid container direction="column">
  <Grid item xs={12}>test</Grid>
</Grid>

Expected Behavior 🤔

Instead, I think height should not be controlled by xs and similar attributes on Grid items. You probably should use width: 100% rather than flex-basis when an item is directly under a column container.

The workaround I am using is to specify the width without the attributes, e.g.

const useStyles = makeStyles(theme => ({
  xs12: {
    width: '100%',
  },
}))
const Comp = () => {
    const classes = useStyles()
    return (
<Grid container direction="column">
  <Grid item className={classes.xs12}>test</Grid>
</Grid>
  )
}

Steps to Reproduce 🕹

https://codesandbox.io/s/mui-bug-for-column-containers-8ztdx

Steps:

1. make a column container with Grid
2. see the item fill the height even though it should not.

Context 🔦

I'm trying to make a simple page with items going down on the page.

Your Environment 🌎

Tech	Version
Material-UI	"@material-ui/core": "^4.5.1",
React	CRA "react": "^16.11.0",
Browser	Chrome 78.0.3904.70
TypeScript	n/a
OS	macOS Version 10.13.6

[oliviertassinari]

@cyrfer As far as I remember, the xs, sm, etc. props don't work with the vertical direction. This issue remembers me of an older one but I can't find it. Do you actually need direction vertical here?

[cyrfer]

@oliviertassinari in fact they cause unexpected results. You clicked the codesandbox link, right?

Yes, frequently I prefer block elements stack in the vertical direction, like most simple websites. Changing to width for column containers seems easy to support. Maybe you can point me in the right direction where I would begin to make a PR.

[oliviertassinari]

@cyrfer I could find the documentation for this problem: https://material-ui.com/components/grid/#direction-column-column-reverse. We should probably be able to make this section more direct, clearer and shorter.

[oliviertassinari]

What about?

diff --git a/docs/src/pages/components/grid/grid.md b/docs/src/pages/components/grid/grid.md
index 465149ea0..01636e70d 100644
--- a/docs/src/pages/components/grid/grid.md
+++ b/docs/src/pages/components/grid/grid.md
@@ -119,12 +119,10 @@ In practice, you can set the `zeroMinWidth` property:

 ### direction: column | column-reverse

-Though the `Grid` component has a `direction` property that allows values of `row`, `row-reverse`, `column`, and `column-reverse`,
-there are some features that are not supported within `column` and `column-reverse` containers.
-The properties which define the number of grids the component will use for a given breakpoint
-(`xs`, `sm`, `md`, `lg`, and `xl`) are focused on controlling width
-and do **not** have similar effects on height within `column` and `column-reverse` containers.
-If used within `column` or `column-reverse` containers, these properties may have undesirable effects on the width of the `Grid` elements.
+The xs, sm, md, lg, and xl props are **not supported** within `direction="column"` and `direction="column-reverse"` containers.
+
+They define the number of grids the component will use for a given breakpoint (focused on controlling width).
+If used, these props may have undesirable effects on the width of the `Grid` item elements.

[cyrfer]

Doc improvements are always appreciated. I think you adequately described the issue now.

Supporting expected behavior seems straight forward to me. I hope we can consider that path also.

[cyrfer]

@oliviertassinari actually, what do you think about changing,

are focused on controlling width and do not have similar effects on height ...

to:

are intended to control width but use flex-basis so they will impact height...

[oliviertassinari]

@cyrfer This sounds good 👍. Do you want to give it a try?

[cyrfer]

@oliviertassinari great!

Yes! Any chance you could point me in the right direction? I'm thinking I need to check when the parent is a column container and make a selector that prefers width over flex-basis (for each of the props xs, sm, etc).

[oliviertassinari]

@cyrfer I meant, for an update of the documentation, not the implementation.

[cyrfer]

@oliviertassinari who can I talk to about fixing the bug?

[oliviertassinari]

@cyrfer At this point, I doubt we can change the Grid component implementation (I suspect we can only add new features), I would fear that a change like this could break people layouts.

[cyrfer]

@oliviertassinari your hesitation is warranted. When I think about it, I cannot imagine who would be depending on a column layout that expresses height using width props. The result does not seem easy to plan around. However, perhaps someone has decided to do that.

Is it possible to control feature flags, perhaps using theme properties? That way developers can "opt into" the better behavior?

[oliviertassinari]

@cyrfer Did you find a way to work around the problem?

[cyrfer]

I feel like my original post was not absorbed.

[joshwooding]

@oliviertassinari great!

Yes! Any chance you could point me in the right direction? I'm thinking I need to check when the parent is a column container and make a selector that prefers width over flex-basis (for each of the props xs, sm, etc).

I would be interested in seeing a proof of concept.

[cyrfer]

@joshwooding I think it would simply look like the codesandbox link.

[sprietNathanael]

Hi @cyrfer

I cannot imagine who would be depending on a column layout that expresses height using width props.

Well, I do! I don't know if I should or if it's a complete misunderstanding of the doc, but I find it very handy in some situations. I don't find where the doc specifies xs as a width prop. It says that it

Defines the number of grids the component is going to use

I use it in vertical container in some of my custom components, and I just had a minor issue with the maxWidth (resolved here ). I imagine some situations where it can be nice to use that if you want to turn your component from horizontal to vertical, but you want to keep proportions of its children.

[sprietNathanael]

@cyrfer
Concerning the wrapping problem, it's the same behaviour than a row container. I can see why it can be bothering, but then why don't you use wrap:'nowrap' in your container? In your codesandbox, it fixes well your issue.

In fact, I don't see why you use a column container. What is your need of using it?
I use a row container to display different size items successively in a row, and if it overflows it, it goes to a second row.
I use a column container to display different size items successively in a column, and if it overflows it, it goes to a second column.

I may be completely wrong though. What do you think @oliviertassinari ?

[sprietNathanael]

@oliviertassinari
What could be really great is to change the maxWidth to maxHeight when the parent container is a column.

[sprietNathanael]

@oliviertassinari
What could be really great is to change the maxWidth to maxHeight when the parent container is a column.

Is there any update on this?

[oliviertassinari]

@sprietNathanael I think that we should improve the documentation around how this is not a supported feature: #18225 (comment). What diff do you have in mind?

[sprietNathanael]

@oliviertassinari Well, as I said earlier, I think it would be really great to support using maxHeight instead of maxWidth when the parent container is set in column direction.
Thus, items behaviour would be the same, whether it's in row or column.

[oliviertassinari]

@sprietNathanael What would the implementation look like?

[sprietNathanael]

Well, I read again some of the comments here and there, and I found again a solution I've used to workaround this issue : #13277 (comment)

I just stumled upon this issue while searching for a reason of this unexpected behaviour. I don't truly understand why breakpoint={1-12} does not make sense with direction="column" for particular uses. I work with a fixed height container and this could be really usefull to use that functionality.

We can work around the problem using CSS selectors.

@oliviertassinari I think this could be a nice workaroud.

I don't know if it's the correct way to handle this, but what worked for me is to apply to the container the following class:

	verticalContainer: {
		'& .MuiGrid-item': {
			maxWidth: 'none',
		},
	},

verticalContainer is a class I add, but it could be replaced by .MuiGrid-container.MuiGrid-direction-xs-column.

This works, but I have to re-implement this css for every component that nedd it. I could use my global theming to do it, but I think it would be really handy to have it implemented internaly in the library.

Moreover, it only bypass maxWidth, but does no set maxHeight instead.

[oliviertassinari]

@sprietNathanael Do you want to send a pull request?