[Card][List] Change sub-components to have fixed gutters

[joshwooding]

• I have followed (at least) the PR section of the contributing guide.

Fixes #10044

• Change theme.mixins.gutters() to 16px paddingLeft and paddingRight

[inv8der]

After upgrading to v3.7.0, I noticed the action element for CardHeader looked a bit funny on larger devices/screens. You can see an example here by resizing the viewport to trigger the 'sm' breakpoint. Since CardHeader now has fixed padding, should the responsive right margin for the action element be removed?

  action: {
    flex: '0 0 auto',
    alignSelf: 'flex-start',
    marginTop: -8,
    marginRight: -12,
    [theme.breakpoints.up('sm')]: {
      marginRight: -20,
    },
  },

[joshwooding]

@inv8der 😮 Thanks for the heads up, looks like this was forgotten. Looks like

should become

  action: {
    flex: '0 0 auto',
    alignSelf: 'flex-start',
    marginTop: -8,
    marginRight: -12,
-    [theme.breakpoints.up('sm')]: {
-      marginRight: -20,
-    },
  },

Do you want to fix this?

[Rokt33r]

@joshwooding There are some bounties in this issue. Please submit this pr to the below link so you can be rewarded! https://issuehunt.io/repos/23083156/issues/10044 (See right bottom of the page)

[oliviertassinari]

@Rokt33r Oh great 👍

[oliviertassinari]

@Rokt33r Should contributors do the same with the other issues? We have closed few issues with a reward but none seems to have been attributed:

[Rokt33r]

@oliviertassinari Yes, they should do it for now.

But I should definitely make it simpler. We're thinking those steps.

1. Make owners can submit other's pr.
2. Implement bot commands so owner can reward without opening issuehunt page.

I'll try to deal with it from the start of the next January. Sorry for making you confused.

[inv8der]

I can submit a pull request to fix this real quick. Sorry, I've been a bit busy this weekend.

[jedwards1211]

Hey guys, as far as I understand this was changed to comply with newer versions of the spec, and I get that, but it caused a breaking layout change in my app after an upgrade that should have been semver-compatible, because I am using theme.mixins.gutters in my app to align various components outside of lists with the list item text. Was the breaking change completely intentional or was it an unexpected consequence?

Are layout changes intended to be postponed for major releases except in rare cases? I hope there will be strict scrutiny of whether layout changes can be considered semver-compatible.

[eps1lon]

Are layout changes intended to be postponed for major releases except in rare cases? I hope there will be strict scrutiny of whether layout changes can be considered semver-compatible.

That is almost impossible to know. Any style we change might cause layout changes. There is no conscious heuristic for mapping css changes to semver. This looked somewhat harmless to me and since it was also a bugfix (not adhering to the spec) it seemed fine. In other cases (e.g. Typography) we opted for a safer route with deprecation.

Seemed like argos did pick up some differences. The diff is not visible anymore though so it's hard to tell what it was. I would agree that this should've been a breaking change then.

Was the breaking change completely intentional

They never are. But the nature of including styles in a library means that any change to the css or DOM structure should be a breaking change. We try to be careful but we're only human.

Could you open an issue outlining the breakage and a fix you've found? That might help others with similar issues. As far as I know you're the first to report it.

[jedwards1211]

Yeah, it definitely is difficult to decide which CSS changes should be considered breaking or not, and I understand there will be mistakes. I'm bringing this up to try to advocate for more strict scrutiny of CSS changes in the future.

Here are some considerations I think should weigh on how to release future CSS changes like this:

• the spec is a little vague on this; the margins were inferred from examples rather than explicit documentation
• the spec changed the gutters in its newer version but again this was not codified in explicit, concrete language AFAIK
• I didn't see any discussion in [Card][ListItem] Follow specification padding left & right  #10044 about which major/minor versions of this repo are intended to track which versions of the spec, but I assume the intent is for only major versions of this project to track spec changes?
• Not complying to the gutters in the Material UI spec isn't as serious as not complying to a spec like TCP or IP. In general there is less problem with putting off MUI spec compliance fixes until the next major release.

From the perspective of these considerations, I don't think think it was an important enough change to justify the risk of breaking layout changes in a minor version.

So I ask you guys will keep this in mind next time a similar situation comes up. If a PR affects appearance in any way, I hope the default assumption will be that it is a breaking change unless there is a very compelling reason to release it sooner (like fixing glitchy behavior, as opposed to complying with a newer version of the spec).

Tomorrow morning I'll create an example sandbox to demonstrate how the change affected me.

[eps1lon]

Not complying to the gutters in the Material UI spec isn't as serious as not complying to a spec like TCP or IP

Sure. I'm using the term "spec" only as a shorthand. They're just guidelines since they're using ambiguous language, don't have document versions or communicate changes etc. When I say "material spec" I mean "material design guidelines". It's just shorter. Sorry for the confusion.

the spec changed the gutters in its newer version but again this was not codified in explicit, concrete language AFAIK

The material guidelines never do that. There is no comprehensive changelog. We can only look at the spec and compare with the implementation

I didn't see any discussion in #10044 about which major/minor versions of this repo are intended to track which versions of the spec,

There is no version of the spec. We loosely refer to the current spec as material v2. Since we don't have access to a list of changes between v1 and v2 we can't decide what version of material-ui is planned to include what changes in the spec. Such a list would be extremely helpful I agree.

So I ask you guys will keep this in mind next time a similar situation comes up.

I already told you that we always do that. We didn't break your code intentionally. You can't expect use to give you any guarantee beyond "we'll try".

I hope the default assumption will be that it is a breaking change unless there is a very compelling reason to release it sooner

This is the dilemma. "implementing material guidelines" means in semver terms that this is our "public API". Therefore any change that goes from "not following the guidelines" to "following the guidelines" can only be considered a bug fix. That is in semver terms. User can and will consider it a breaking change regardless which is where we have to make a judgment call. It seems like we have a decent track record in doing so. This time we didn't.

We might consider batching any layout related changes (size, display, position etc) into a single breaking change. I'm not sure it's better to bundle them into a single version though. Maybe we should add warnings to changelog entries if we do change layout that include a patch to revert to the previous behavior. This would allow devs to examine the impact better.

[jedwards1211]

You're right about the track record, I don't give you guys enough credit. I can't think of any other unexpected breaking changes I've encountered like this 🎉

[jedwards1211]

Okay my sandbox demonstration is in #14751.
I was blowing this issue out of proportion, mainly because I'm a semver purist; really it's just a minor annoyance, because it won't be hard to fix this in my own code. So I'll be nice if you guys decide supporting/documenting a futureproof API for matching list gutters is out-of-scope for this project. But in case you think it's worth it, the sandbox should demonstrate the challenge.