Avoid duplication of container breakpoints

[k-utsumi]

ref.

• https://github.com/twbs/bootstrap/blob/v4.5.0/dist/css/bootstrap.css#L550-L572
• https://github.com/twbs/bootstrap/blob/v4.5.0/dist/css/bootstrap.css#L582-L604

[ffoodd]

Nioce finding, thanks :)

Seems to work great, preview: https://deploy-preview-30969--twbs-bootstrap.netlify.app/docs/4.5/examples/grid/

I think this is because we use breakpoint-infix() in our nested loops, which returns an empty string for the smaller breakpoint (thus .container is always present):

bootstrap/scss/_grid.scss

Line 33 in 3caea1c

.container#{breakpoint-infix($name, $grid-breakpoints)} {

FWIW, if we remove this inclusion, the whole make-container-max-widths() mixin becomes unused.

@k-utsumi, could you please:

1. remove the code instead of commenting it (usually a bad idea to commit commented code, you have Git to get it back if needed)
2. drop the whole mixin in scss/mixins/_grid.scss

@twbs/css-review I think this also affects v5, yet it would not be backportable since containers and grid have been split on master. So if there's an agreement here, it'd require another PR against master.

[MartijnCuppens]

@twbs/css-review I think this also affects v5, yet it would not be backportable since containers and grid have been split on master. So if there's an agreement here, it'd require another PR against master.

Yup, I agree

[k-utsumi]

@ffoodd
Thank you for confirmation🙋‍♂️
I have updated. Is it okay?

[ffoodd]

Yes! Thanks :)

[ffoodd]

@MartijnCuppens In fact, our .container behaves exactly like our .container-sm. Feels a bit weird to me, I didn't notice.

[k-utsumi]

Built code diff

[k-utsumi]

I think [sm ~ xl] container isn't useful.
It's not something I say here, but I'm glad if the code is like this.

.container,
.container-[sm ~ xl] { max-width: sm-width }
@include media-breakpoint-up(md) {
  .container, 
  .container-[md ~ xl] { max-width: md-width }
}
@include media-breakpoint-up(lg) {
  .container, 
  .container-[lg ~ xl] { max-width: lg-width }
}
@include media-breakpoint-up(xl) {
  .container, 
  .container-[xl] { max-width: xl-width }
}

[ffoodd]

Agreed, .container-xl doesn't seem useful here.

FWIW we cannot drop them in v4 since it would break things. However it's probably to be considered for v5.

[mdo]

Not sure I follow—what's the problem with .container-xl?

[k-utsumi]

After all, My suggestion's .container and .container-xl are exactry the same.

If set size xl at variables with size name, I don't mind.

[mdo]

The containers definitely do different things. At the xxl breakpoint, every container is the same size, but they do behave differently—they're 100% wide until their breakpoint is reached an the max-width kicks in. .container and .container-sm are also the same, yes. Not sure it's worth nixing one or the other.

[mdo]

Btw, this will need to be changed in v5's master branch as well since we've brought that feature forward.

[k-utsumi]

The current behavior of [sm ~ xl] is difficult to explain to designers and workers of other layers, and It is difficult to imagine the usage scene, I think.
Since there were many times when I used some max-width container, I made a suggestion.

Behavior sample:
https://jsfiddle.net/c487rgvm/1/

[mdo]

That would indeed be a large change in behavior. The current behaviors come from #25631 and related PRs. They were highly requested features, so for the time being we'll stick with it.

[k-utsumi]

Thanks, maybe I understood that behavior.
I thout that changing to the code like this will make it easier to understand.

HTML

<div class="container-fluid container-[sm ~ xl]-fixed">
  ...
</div>

CSS

.container,
.container-fluid {
  width: 100%;
  padding-right: 1rem;
  padding-left: 1rem;
  margin-right: auto;
  margin-left: auto;
}
@media (min-width: $grid-breakpoint[sm]) {
  .container, .container-[sm]-fixed {
    max-width: $container-max-width[sm];
  }
}
@media (min-width: $grid-breakpoint[md ... xl]) {
  .container, .container-[sm ... xl]-fixed {
    max-width: $container-max-width[md ... xl];
  }
}

I may not get the approval.
The question is that the class name is difficult to understand.

All I have to do is read the documentation...😅
Sorry, and thank you for your confirmation.

[XhmikosR]

@mdo is this ready to be merged?