Skip to content

Avoid duplication of container breakpoints#30969

Merged
XhmikosR merged 5 commits into
twbs:v4-devfrom
k-utsumi:patch-1
Jun 13, 2020
Merged

Avoid duplication of container breakpoints#30969
XhmikosR merged 5 commits into
twbs:v4-devfrom
k-utsumi:patch-1

Conversation

@k-utsumi

@k-utsumi k-utsumi commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

@k-utsumi k-utsumi requested a review from a team as a code owner June 5, 2020 04:53
@ffoodd

ffoodd commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

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):

.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 MartijnCuppens changed the title 🔥 Avoid duplication of container breakpoints Avoid duplication of container breakpoints Jun 5, 2020
@MartijnCuppens

Copy link
Copy Markdown
Member

@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

k-utsumi commented Jun 5, 2020

Copy link
Copy Markdown
Contributor Author

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

@ffoodd

ffoodd commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

Yes! Thanks :)

@ffoodd

ffoodd commented Jun 5, 2020

Copy link
Copy Markdown
Contributor

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

Comment thread scss/_grid.scss
@include make-container-max-widths();
}

.container,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Built code diff
image

@k-utsumi

k-utsumi commented Jun 6, 2020

Copy link
Copy Markdown
Contributor Author

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

ffoodd commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

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

mdo commented Jun 11, 2020

Copy link
Copy Markdown
Member

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

@k-utsumi

Copy link
Copy Markdown
Contributor Author

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

mdo commented Jun 11, 2020

Copy link
Copy Markdown
Member

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.

Screen Shot 2020-06-11 at 2 31 43 PM

@mdo

mdo commented Jun 11, 2020

Copy link
Copy Markdown
Member

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

@k-utsumi

k-utsumi commented Jun 12, 2020

Copy link
Copy Markdown
Contributor Author

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

mdo commented Jun 12, 2020

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

@mdo is this ready to be merged?

@XhmikosR XhmikosR merged commit c3572ce into twbs:v4-dev Jun 13, 2020
@k-utsumi k-utsumi deleted the patch-1 branch June 15, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants