[breakpoint] Down properties are now inclusive #9632

kgregory · 2017-12-27T18:46:24Z

xsDown, smDown, mdDown, lgDown, and xlDown match the behavior of their Up counterparts and are now inclusive of the specified breakpoint. This is consistent between the CSS and JS implementation of this component.

Additionally, theme.breakpoints.down has been altered to match this behavior. This function is now consistent with its up and only counterparts.

The implementation of isWidthDown has been altered so that its default behavior is to be inclusive (this was hinted in an associated comment, but was not the case).

Documentation has been updated, as well as internal dependence on the down or isWidthDown functions.

The decision to move forward with this PR was made following a conversation with @oliviertassinari and @rosskevin, which resulted in a conclusion that it would be better to deal with this inconsistency and the breaking changes its solution would require while we are in beta.

Breaking changes

createBreakpoints.down() is now inclusive of the specified breakpoint
isWidthDown() is now inclusive of the specified breakpoint by default
<Hidden /> will include the breakpoints associated with its Down properties regardless of whether CSS or JS is used.

@oliviertassinari

xsDown, smDown, mdDown, lgDown, and xlDown match the behavior of their *Up* counterparts and are now inclusive of the specified breakpoint. This is consistent between the CSS and JS implementation of this component. Additionally, `theme.breakpoints.down` has been altered to match this behavior. This function is now consistent with its `up` and `only` counterparts. Documentation has been updated, as well as internal dependence on the `down` function. The implementation of isWidthDown has been altered so that its default behavior is to be inclusive (this was hinted in an associated comment, but was not the case). The decision to move forward with this PR was made following a conversation with @oliviertassinari and @rosskevin, which resulted in a conclusion that it would be better to deal with this inconsistency and the breaking changes its solution would require while we are in beta. These are the following breaking changes: - 'createBreakpoints.down' is now inclusive of the specified breakpoint - `isWidthDown` is now inclusive of the specified breakpoint by default - `Hidden` will include the breakpoints associated with its *Down* properties regardless of whether CSS or JS is used.

oliviertassinari

It's a very good start :).
Don't forget to update this section too https://material-ui.com/layout/hidden/#how-it-works

oliviertassinari · 2017-12-27T20:10:56Z

src/Hidden/HiddenJs.spec.js

@@ -136,7 +135,7 @@ describe('<HiddenJs />', () => {
    });

    describe('down', () => {
-      isVisible(['xs', 'sm', 'md', 'lg', 'xl'], 'Down', 'xl');


Missing isHidden with xl.

This has been dealt with

oliviertassinari · 2017-12-27T20:14:25Z

src/styles/createBreakpoints.js

+
+    if (endIndex === keys.length) {
+      // xl down applies to all sizes
+      return up('xs');


Humm, maybe we should just warn. What's the point of using?

root: { color: 'red', [theme.breakpoints.down('xl')]: { color: 'blue', }, },

Isn't it equivalent to?

root: { color: 'blue', },

Aside of the warning. I have my doubt about this new logic.

The same could be said for xsUp, which applies to all sizes (min-width: 0px), which is why I did it this way.

Ok, let's not add a warning as someone might want to do it this way to beat specificity. The logic is correct 👍

oliviertassinari · 2017-12-27T20:16:08Z

src/styles/createBreakpoints.spec.js

+    });
+
+    it('should apply to no sizes if unknown size is specified', () => {
+      assert.strictEqual(breakpoints.down('xxl'), '@media (max-width:-0.05px)');


Humm. Do we have a test with a number as first argument? I'm not sure we want to test xxl 😁 .

oliviertassinari · 2017-12-27T20:17:45Z

src/styles/createBreakpoints.spec.js

@@ -21,8 +21,20 @@ describe('createBreakpoints', () => {
  });

  describe('down', () => {
-    it('should work', () => {
-      assert.strictEqual(breakpoints.down('md'), '@media (max-width:959.95px)');
+    it('should work for xs', () => {


Using sm instead of xs would have allowed showing that the assertion don't change.

oliviertassinari · 2017-12-27T20:21:23Z

TL;DR

This pull-request is about the intuitiveness of the API. Always using an inclusive behavior instead of having to remember what's inclusive and what's exclusive. ~~It's the same approach used by Bootstrap.~~

- Updated hidden doc - Added an isHidden test - createBreakpoints down will use the key if it is an unspecified breakpoint - cleaned up createBreakpoints tests

kgregory · 2017-12-27T21:15:09Z

Thanks for the review @oliviertassinari, let me know if there's anything further on this one!

oliviertassinari · 2017-12-28T12:31:40Z

@kgregory You ganna need to run yarn docs:api to fix the CI.

oliviertassinari

Let's cross our fingers and wait for people feedback 👌🏻 once it's released.

oliviertassinari · 2017-12-28T13:55:56Z

- AppDrawer and ResponsiveDrawer were using *Down properties that needed to be shifted down one breakpoint due to the changes in this PR - ResponsiveDrawer was reported in Argos - AppDrawer was not reported in Argos - BreakpointDown was reported as a regression, but the current behavior is as expected

…regory/material-ui into feature/inclusive-breakpoints

kgregory · 2017-12-28T17:15:43Z

AppDrawer and ResponsiveDrawer were using *Down properties that needed to be shifted down one breakpoint due to the changes in this PR
ResponsiveDrawer was reported in Argos, but AppDrawer was not
BreakpointDown was reported as a regression, but the current behavior is as expected

kgregory · 2017-12-28T18:59:03Z

@oliviertassinari The argos difference in BreakpointDown is the expected behavior, given this change. mdDown will hide the children for a medium viewport. How can I get this test to pass?

oliviertassinari · 2017-12-28T19:18:32Z

How can I get this test to pass?

@kgregory You need to login and accept the build. I'm doing it now :).

oliviertassinari · 2017-12-28T22:11:24Z

Let's go. I hope it won't affect people too much 🎲 . @kgregory Good job! :)

kgregory added 2 commits December 27, 2017 13:32

Merge branch 'v1-beta' into feature/inclusive-breakpoints

736d655

rosskevin added the breaking change label Dec 27, 2017

oliviertassinari reviewed Dec 27, 2017

View reviewed changes

oliviertassinari modified the milestones: v1.0.0, v1.0.0-prerelease Dec 27, 2017

oliviertassinari added the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 27, 2017

oliviertassinari assigned kgregory Dec 27, 2017

[Hidden] address PR notes

405bcf9

- Updated hidden doc - Added an isHidden test - createBreakpoints down will use the key if it is an unspecified breakpoint - cleaned up createBreakpoints tests

[Hidden] API docs update

761f6ff

oliviertassinari approved these changes Dec 28, 2017

View reviewed changes

oliviertassinari removed the PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI label Dec 28, 2017

oliviertassinari changed the title ~~[Hidden] down properties are now inclusive~~ [breakpoint] Down properties are now inclusive Dec 28, 2017

kgregory added 3 commits December 28, 2017 12:06

Merge branch 'v1-beta' into feature/inclusive-breakpoints

7c235db

Merge branch 'feature/inclusive-breakpoints' of https://github.com/kg…

958799a

…regory/material-ui into feature/inclusive-breakpoints

[Hidden] bump .size-limit for overhead to 24.4 from 24.3

0e5f3b8

oliviertassinari added the PR: accepted label Dec 28, 2017

oliviertassinari merged commit b72e771 into mui:v1-beta Dec 28, 2017

kgregory deleted the feature/inclusive-breakpoints branch December 28, 2017 22:14

oliviertassinari mentioned this pull request Apr 25, 2019

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

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[breakpoint] Down properties are now inclusive #9632

[breakpoint] Down properties are now inclusive #9632

kgregory commented Dec 27, 2017 •

edited by oliviertassinari

oliviertassinari left a comment •

edited

oliviertassinari Dec 27, 2017

kgregory Dec 28, 2017

oliviertassinari Dec 27, 2017 •

edited

kgregory Dec 27, 2017

oliviertassinari Dec 27, 2017

oliviertassinari Dec 27, 2017

oliviertassinari Dec 27, 2017

oliviertassinari commented Dec 27, 2017 •

edited

kgregory commented Dec 27, 2017

oliviertassinari commented Dec 28, 2017

oliviertassinari left a comment

oliviertassinari commented Dec 28, 2017

kgregory commented Dec 28, 2017

kgregory commented Dec 28, 2017

oliviertassinari commented Dec 28, 2017

oliviertassinari commented Dec 28, 2017 •

edited

[breakpoint] Down properties are now inclusive #9632

[breakpoint] Down properties are now inclusive #9632

Conversation

kgregory commented Dec 27, 2017 • edited by oliviertassinari

Breaking changes

oliviertassinari left a comment • edited

Choose a reason for hiding this comment

oliviertassinari Dec 27, 2017

Choose a reason for hiding this comment

kgregory Dec 28, 2017

Choose a reason for hiding this comment

oliviertassinari Dec 27, 2017 • edited

Choose a reason for hiding this comment

kgregory Dec 27, 2017

Choose a reason for hiding this comment

oliviertassinari Dec 27, 2017

Choose a reason for hiding this comment

oliviertassinari Dec 27, 2017

Choose a reason for hiding this comment

oliviertassinari Dec 27, 2017

Choose a reason for hiding this comment

oliviertassinari commented Dec 27, 2017 • edited

TL;DR

kgregory commented Dec 27, 2017

oliviertassinari commented Dec 28, 2017

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari commented Dec 28, 2017

kgregory commented Dec 28, 2017

kgregory commented Dec 28, 2017

oliviertassinari commented Dec 28, 2017

oliviertassinari commented Dec 28, 2017 • edited

kgregory commented Dec 27, 2017 •

edited by oliviertassinari

oliviertassinari left a comment •

edited

oliviertassinari Dec 27, 2017 •

edited

oliviertassinari commented Dec 27, 2017 •

edited

oliviertassinari commented Dec 28, 2017 •

edited