Change media-breakpoint-down implementation

[MartijnCuppens]

media-breakpoint-down() always used the next breakpoint, so if you want to target viewports smaller than lg (992px) you needed to use:

.selector {
  // Below lg breakpoint:
  @include media-breakpoint-down(md) {
    display: none;
  }
}

I've made another codepen to illustrate the problem:
https://codepen.io/MartijnCuppens/pen/pMNjaN?editors=1100

In this PR, I've changed the implementation to use the breakpoint itself. I've also dropped the media-breakpoint-only() mixin since we do not use the "breakpoint zone" approach like we did in v3.

The new implementation looks like this:
https://codepen.io/MartijnCuppens/pen/MNBqjY?editors=1100

To simplify reviews: this is the difference in generated css, some blocks are just shifted:
https://www.diffchecker.com/2ZUTxc1P (updated June 14th)

[XhmikosR]

Can't we keep the only media-breakpoint-only() mixin? I still think it's useful.

[MartijnCuppens]

Ok, I just restored media-breakpoint-only().

[XhmikosR]

LGTM but please wait for another approval from @twbs/css-review

[XhmikosR]

@MartijnCuppens this needs a rebase

@twbs/css-review this needs a review please :)

[XhmikosR]

@ysds @mdo does this LGTY?

[XhmikosR]

@ysds @mdo friendly ping :)

[ysds]

I'm understanding that the current specification is based on device size: https://getbootstrap.com/docs/4.4/layout/overview/#responsive-breakpoints

The suggestion of this PR is to change it to breakpoint based. I summarized my understanding in the following diagram:

Neither theory is wrong, but the current specification is bit complex when considered in the code base. Actually I’ve felt confusing before.

I have no objection to change, but the only concern is incompatibility.

@mdo Thought?

[voltaek]

I understand the reasoning for the change, and while I think it would take a bit to get used to thinking about how these work from the new perspective, I agree with @ysds that compatibility is a concern here, as anyone upgrading to v5 - or even folks used to how v4 worked - might not immediately notice the slight change to how their breakpoints are working when compiling with v5.

One possible solution to avoid potential subtle incompatibility issues would be to rename the mixins as part of the change, forcing users to address the change. Something along these lines:
media-breakpoint-down -> media-breakpoint-below
media-breakpoint-between -> media-breakpoint-within
media-breakpoint-up -> media-breakpoint-above

I happen to also think these names better reflect the fact that they're operating on a breakpoint as a specific value rather than a range, but that could just be me thinking that.

[MartijnCuppens]

I like that approach, @voltaek. Thoughts @ysds @mdo?

[XhmikosR]

below and above might be better but within, I'm not so sure.

BTW, being this will only land in v5, it can certainly be a breaking change, and that's OK. We just need to remember to highlight it.

[voltaek]

To replace between, the first two that come to mind are within or inside, but I'm sure there are other names we could come up with.

If we can't come up with (or agree on) a better name to replace between, we could just change the other two to have names like above and below, as between can just as easily be thought to apply to the breakpoint values themselves as it applies to their ranges, in my opinion. Hopefully changing those two would expose incompatibility issues for most cases, though it would be nice to have a new name for all three in order to have the switch over be as air tight in that regard as possible.

[ysds]

Also possible to change the head part e.g. mq-breakpoint-.

[MartijnCuppens]

@ysds' proposal feels like a good idea, so than we have:

• mq-breakpoint-below
• mq-breakpoint-between
• mq-breakpoint-above

I'll change it within a couple of days.

[XhmikosR]

I wouldn't go with this approach personally.

Anyway, better wait for @mdo's feedback

[XhmikosR]

@MartijnCuppens can you fix the conflicts?

@mdo: friendly ping

[mdo]

For a refresher on the breakpoints, definitely read the docs at https://getbootstrap.com/docs/4.4/layout/overview/#responsive-breakpoints as @ysds says. I might need to reread them a few more times myself lol. Here's a snapshot comparison of each type of media query mixin we have today, and it's CSS equivalent.

@include media-breakpoint-up(sm) { ... }
@media (min-width: 576px) { ... }

@include media-breakpoint-down(sm) { ... }
@media (max-width: 767.98px) { ... }

@include media-breakpoint-only(sm) { ... }
@media (min-width: 576px) and (max-width: 767.98px) { ... }

@include media-breakpoint-between(md, xl) { ... }
@media (min-width: 768px) and (max-width: 1199.98px) { ... }

I think @ysds got one detail wrong in the v4 diagram—media-breakpoint-between doesn't include second breakpoint, it goes up to that second breakpoint, but doesn't include that grid tier.

With this PR in particular, I don't think this is a behavior we should change in v5. The fundamental basis for these mixins is that media-breakpoint-down is max-width and media-breakpoint-up is min-width.

I could see us renaming these, or perhaps providing aliases (is that a thing?), to be much more descriptive. Something like this maybe?

• media-min(sm) or media-min-width(sm) (min-width: 576px)
• media-max(lg) or media-max-width(lg) (max-width: 991.98px)
• media-only(md) (768px – 991.98px)
• media-between(sm, lg) (576px – 991.98px, which is sm-md and md-lg, but not lg-xl)

Nothing but the names would change here though as I understand it. Thoughts?

[ysds]

I think @ysds got one detail wrong in the v4 diagram—media-breakpoint-between doesn't include second breakpoint, it goes up to that second breakpoint, but doesn't include that grid tier.

My diagram show it (include or not) with black and white circles 😉

v4:

@include media-breakpoint-between(sm, md) { ... }

is

@media (min-width: 576px) and (max-width: 991.98px) { ... }

[MartijnCuppens]

Ok, switched this to @mdo s suggestions. Wouldn't work with aliases, it 'll just pollute our code base.

[XhmikosR]

@MartijnCuppens this needs a rebase. @mdo let us know what you think

[ysds]

I prefer up/down since it makes intuitive sense. The min/max always bit confuse me.

#28683 will add classes such as .modal-fullscreen-md-down. And our media breakpoint mixins are might used frequently, so these names should be consistent. However, .modal-fullscreen-md-max will probably confuse many people. .modal-fullscreen-md-down is much easier to understand.

[MartijnCuppens]

I've removed the renames and just continued with the initial idea of using the the current bp instead of the next. If the renaming of mixins is needed, we could cover this in another PR.

It seems this way of using the media-breakpoint-down was already documented in https://twbs-bootstrap.netlify.app/docs/4.3/layout/breakpoints/#between-breakpoints and https://twbs-bootstrap.netlify.app/docs/4.3/layout/breakpoints/#between-breakpoints.

@twbs/css-review, could you review this?

[mdo]

@MartijnCuppens Sorry for late approval, I say we bite the bullet on this and ship it in v5. Any chance for a rebase?

[mdo]

Thanks!