[Breakpoints] functions down() and between() adds +1 to index

[b5imply]

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Question

Why does the breakpoints functions down(key) and between(start, end) iterate the key and end +1 ?

Beheviour

If I want to style something below, say md (960px) for example, it adds +1 to the index and gets lg (1280px) instead. Dosen't down(key) means <=key?

between(start, end) always adds 1 to the end index. So if you want to style something between 960px and 1280px I need to call between('md', 'md'); which adds +1 to the end index of the last argument which gives @media (min-width: 960px) (max-width: 1280px ).

Link to reproduction

Link: https://codesandbox.io/s/20w4v142lj

1. Adjust window to see box color change based on width

Context

From createBreakpoints.js in @material-ui/core/styles/:

function  down(key) {
    var  endIndex  =  keys.indexOf(key) +  1; // HERE
    var  upperbound  =  values[keys[endIndex]];
    
    if (endIndex  ===  keys.length) {
	    // xl down applies to all sizes
	    return  up('xs');
    }
    
    var  value  =  typeof  upperbound  ===  'number'  &&  endIndex  >  0  ?  upperbound  :  key;
    return  "@media (max-width:".concat(value  -  step  /  100).concat(unit, ")");
}

The same applies to the between(start, end) function

Environment

Tech	Version
Material-UI	v3.3.2
React	v16.5.2
Browser	Chrome v70.0.3538.77

[guix77]

After reading I still find it very strange to just not implement in the natural way which is

theme.breakpoints.down('md')

===

@media (max-width: 960px)

Why make it simple when it can be complex ?

Anyways, some examples could be added for clarity or maybe improve again the doc like for instance:

theme.breakpoints.down('md')

= matches screen widths less than and including the md screen widths range, which is (960 - 1279 px)

===

@media (max-width: 1279px)

Nothing strange, here, really ? ;-)

[oliviertassinari]

@guix77 What would be a correct logic?

[guix77]

Maybe this ?

theme.breakpoints.down('sm') => max-width: (sm-1) px
theme.breakpoints.downOrEqual('sm') => max-width: sm px
theme.breakpoints.up('sm') => min-width: (sm+1) px
theme.breakpoints.upOrEqual('sm') => min-width: sm px
theme.breakpoints.between('md', 'sm') => (min-width: md px and max-width: sm px)

[oliviertassinari]

Thank you, everybody, for the feedback on this issue. The best proposal I can come up with is to change the mental model behind breakpoints. Instead of considering breakpoints as values and ranges. It will be simpler to consider them as values exclusively. In this context, theme.breakpoints.down(x) will match the screen width strictly below the breakpoint value of x. The between helper would follow the same rule. The only case that might be confusing is when .only(x) is used. It would be equivalent to .between(x, breakpoint after x).
In practice, it's very close to the approach Bootstrap uses and what the authors of #13448 and #17875 propose. Hopefully, it's more intuitive.

This is a breaking change. We should wait for v5 to consider implementing it. The impact might be important but is limited by the fact that the usage of .up(x) should be significantly more frequent (our demos + mobile-first approach) and won't be impacted by the proposal.

[aditya81070]

@oliviertassinari Thanks a lot for this and importantly using hooks in v4. The material-UI is now more easy to use because of hooks.

[oliviertassinari]

I was wrong, Bootstrap v4 and Material-UI v4 have the same breakpoint behavior (I don't recall, but that might have influenced the current behavior).
However, they might apply the point this issue support in v5: twbs/bootstrap#29148.

[adybaby]

I've overwritten up, down, and between in theme.breakpoints as follows:

breakpoints: {
  get down() {
    return (key) => `@media (max-width:${this.values[key] - 0.5}px)`;
  },
  get up() {
    return (key) => `@media (min-width:${this.values[key] + 0.5}px)`;
  },
  get between() {
    return (keyA, keyB) =>
      `@media (min-width:${this.values[keyA]}px) and (max-width:${this.values[keyB] - 0.5}px)`;
  },
},

It seems to be working fine so far (and far less confusing, to my mind), but can anyone see any problems with this down the road?

[VenomFate-619]

When it will be solved , I also facing this issue

[oliviertassinari]

@VenomFate-619 It's already solved, you can upgrade to v5.0.0-beta.5.