Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[breakpoints] Fix down function, eliminate overlap #6504

Merged
merged 4 commits into from Apr 6, 2017
Merged

[breakpoints] Fix down function, eliminate overlap #6504

merged 4 commits into from Apr 6, 2017

Conversation

kgregory
Copy link
Member

@kgregory kgregory commented Apr 5, 2017

Consider the following:

   [theme.breakpoints.down('sm')]: {
      content: {
         margin: 0,
      },
   },
   [theme.breakpoints.only('sm')]: {
      content: {
         margin: 12,
      },
   },
  • The first rule states that the content class will apply a margin of 0 when the viewport ranges in size from 0-600.
  • The second rule states that the content class will apply a margin of 12 when the viewport ranges in size from 600-959.

There is a 1px overlap because down sets a max-width equal to the size defined for the specified breakpoint. This is inconsistent with only which defines a max-width which is 1px less than the size defined for the next breakpoint.

This has been submitted as issue #6503.

This PR fixes the down function so that the max-width is set to 1px less than the size defined for the specified breakpoint and updates the test in the spec so that this change is accounted for.

  • PR has tests / docs demo, and is linted.
  • Commit and PR titles begin with [ComponentName], and are in imperative form: "[Component] Fix leaky abstraction".
  • Description explains the issue / use-case resolved, and auto-closes the related issue(s) (http://tr.im/vFqem).

- Should be consistent with the only function and set a max-width of 1 pixel less than the size of the specified breakpoint so that there isn't an overlap of 1 pixel.
- use step instead of 1px
- Removing a pixel is too much.  In Chrome and Edge inspectors, a viewport of 599 is actually 599.2.  We only need to ensure that it is less than 600.
- Subtract 1 percent of our step in Down and Between.
@muibot muibot added PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI and removed PR: Needs Review labels Apr 5, 2017
@kgregory
Copy link
Member Author

kgregory commented Apr 5, 2017

Test case for the docs site (stylesheet from /src/components/AppContent.js):

export const styleSheet = createStyleSheet('AppContent', (theme) => {
  return {
    content: theme.mixins.gutters({
      paddingTop: 80,
      flex: '1 1 100%',
      maxWidth: '100%',
      margin: '0 auto',
    }),
    [theme.breakpoints.between('sm', 'md')]: {
      content: {
        margin: 80,
        background: '#f00',
      },
    },
    [theme.breakpoints.down('sm')]: {
      content: {
        margin: 40,
        background: '#00f',
      },
    },
    [theme.breakpoints.only('sm')]: {
      content: {
        margin: 60,
        background: '#0f0',
      },
    },
  };
});

Removing one pixel (as defined in the step variable) is too much because some viewports render with a fraction of a pixel. For example, Chrome and Edge inspectors render a 599px viewport as 599.2, which is greater than the max-width we're defining. We only need to be less than the size defined for the specified breakpoint.

With the example above, setting a viewport to 1279px in Chrome's inspector should apply the between sm and md rules, but they are not getting applied because the actual width is greater: 1279.2px.

To fix this, I've subtracted 1% of our step. This would set the max-width to 1279.99 for the between sm, md rules and 599.99 for the down sm rules, ensuring that edge case viewports are properly styled.

- update test to check for new max widths
- max-width will be 0.01 pixel less than the next breakpoint
@oliviertassinari oliviertassinari added next and removed PR: Needs Review PR: needs revision The pull request can't be merged. More details is available on the code review or fails in the CI labels Apr 5, 2017
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Thanks for giving a shot at this issue. The overall logic looks good to me. I have been looking at what bootstrap is doing for some context: https://github.com/twbs/bootstrap/blob/v4-dev/scss/mixins/_breakpoints.scss#L38.

@oliviertassinari
Copy link
Member

Thanks.

@oliviertassinari oliviertassinari changed the title [breakpoints] fix down function, eliminate 1px overlap with specified breakpoint [breakpoints] Fix down function, eliminate overlap Apr 6, 2017
@zannager zannager added the docs Improvements or additions to the documentation label Mar 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants