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

Support an API for matching the gutters of lists #14751

Closed
jedwards1211 opened this issue Mar 5, 2019 · 7 comments
Closed

Support an API for matching the gutters of lists #14751

jedwards1211 opened this issue Mar 5, 2019 · 7 comments
Labels
component: list This is the name of the generic UI component, not the React module!

Comments

@jedwards1211
Copy link
Contributor

jedwards1211 commented Mar 5, 2019

In my app I have a lot of views with <List>s and other components inside of a <Paper> component.

Example Sandbox

I need to make the gutters around the other components always match the gutters of the <List>, like this:

image

I originally solved this problem by using theme.mixins.gutters() on the div surrounding the Input (or other content), since ListItem used to use theme.mixins.gutters() itself.

But since #13788 was merged, ListItem no longer uses theme.mixins.gutters(), so after a semver-compatible upgrade, the alignment in my app is broken on wide screens (since theme.mixins.gutters() applies wider gutters above a certain screen width, but ListItem no longer does):

image

Material design guidelines may go back to screensize-dependent gutters in the future, who knows?

Maybe I should just guard against this on my side by controlling ListItem margins myself in theme overrides.

But, I bet aligning components with ListItem content is a fairly common problem? So devs may benefit from public API in this project for matching the margins used in ListItem, so that if ListItem gutters are changed again in the future, devs don't have to update their surrounding components after upgrading Material-UI to fix the margins.

@eps1lon
Copy link
Member

eps1lon commented Mar 5, 2019

The alignment was accidental previously. They have a fixed padding that is not responsive. I don't know how we should communicate what padding/margin is supposed to be responsive.

Suggested fix: https://codesandbox.io/s/5wz8m782kp

Just to clarify: This was considered a bug fix since we didn't follow the material design guidelines.

There's no telling if the Material design guidelines will go back to screensize-dependent gutters in the future...

If we can identify that the spec was changed we should only change the implementation in a breaking change like we did with the typography. This wasn't the case with the ListItem measurements.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Mar 5, 2019

This gets down to the semantics of what's considered a bug and the gray areas of semver, but I see this "bug fix" as a different situation than fixing a glitch that is causing people's apps to not work. In this case people's apps weren't matching the arbitrary guidelines, but their apps' layout was at least self-consistent. I'm assuming (maybe I'm wrong) that most people coded their layout according to the behavior of this library rather than the design guidelines. So I question whether a minor noncompliance with the guidelines like this should be released as a patch/minor change like fixes for broken/glitchy behavior.

@jedwards1211
Copy link
Contributor Author

jedwards1211 commented Mar 5, 2019

I think I will probably solve this in my project by simply overriding theme.mixins.gutters() to be fixed to 16px everywhere, in case any other components in this project still use responsive padding (I wonder if default table padding in this project still matches default list padding in this project at all screen sizes?)

@kgregory
Copy link
Member

kgregory commented Mar 5, 2019

This gets down to the semantics of what's considered a bug and the gray areas of semver, but I see this "bug fix" as a different situation than fixing a glitch that is causing people's apps to not work. In this case people's apps weren't matching the arbitrary guidelines, but their apps' layout was at least self-consistent.

The default behavior is to adhere to the Material Design specification. This component was not doing that. Instead, it was producing an incorrect result and was behaving in unintended ways.

I think I will probably solve this in my project by simply overriding theme.mixins.gutters() to be fixed to 16px everywhere, in case any other components in this project still use responsive padding.

I don't think this is a good approach. You should eliminate inappropriate use of the mixin and apply a fixed padding as needed.

@jedwards1211
Copy link
Contributor Author

@kgregory but to match components that have a responsive padding? How else can I make it so that I never have to worry about horizontal padding again but for everything to use the same horizontal padding?

@oliviertassinari
Copy link
Member

oliviertassinari commented Mar 9, 2019

@jedwards1211 I would simplify wrap the text field within a ListItem with the component prop that makes sense in the DOM context it's in.

@jedwards1211
Copy link
Contributor Author

Personally that's unappealing to me because it's an unsemantic use of ListItem, similar to how people used to use tables for layout everywhere, though not as bad. Fortunately though, now that the horizontal padding of most things is fixed to 16px, it's not really a big deal how I solve the alignment.

@oliviertassinari oliviertassinari added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Dec 20, 2022
@zannager zannager added component: list This is the name of the generic UI component, not the React module! and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: list This is the name of the generic UI component, not the React module!
Projects
None yet
Development

No branches or pull requests

5 participants