Skip to content

(PDS-442) Add breakpoint tools to sass-variables #218

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

Merged
merged 1 commit into from
Feb 21, 2020

Conversation

caseywilliams
Copy link
Contributor

@caseywilliams caseywilliams commented Feb 13, 2020

Adds variables and mixins to the sass-variables package to allow for
configuring and responding to different breakpoints.

There are three breakpoints: small, medium, and large:

  • Portrait-mode phones tend to be smaller than 'small'
  • Landscape-mode phones tend to be between 'small' and 'medium'
  • Tablets and small browser windows tend to be between 'medium' and 'large'
  • Browser windows and some landscape-mode phones tend to be larger
    than 'large'

Default breakpoints can be overridden by defining your own
@breakpoints map in your stylesheets before importing
sass-variables.

Two mixins are included:

  • respond-above applies styles to widths greater than the given size
  • respond-below applies styles to widths below the given size

@caseywilliams caseywilliams requested a review from a team as a code owner February 13, 2020 21:42
@caseywilliams
Copy link
Contributor Author

caseywilliams commented Feb 13, 2020

I tested these in the new forge repo - everything seems to work, including overriding the breakpoint definitions locally. Here's a quick way to see them functioning:

<div class="boxes">
	<div id="one" />
	<div id="two" />
</div>
@import '~@puppet/sass-variables/index';

.boxes {
  div {
    height: 200px;
    width: 200px;
    background-color: lightblue;
  }

  #one {
    @include respond-above(small) {
      background-color: palegoldenrod;
    }
    @include respond-above(medium) {
      background-color: lightsalmon;
    }
    @include respond-above(large) {
      background-color: pink;
    }
  }

  #two {
    @include respond-below(large) {
      background-color: pink;
    }
    @include respond-below(medium) {
      background-color: lightsalmon;
    }
    @include respond-below(small) {
      background-color: palegoldenrod;
    }
  }
}

@caseywilliams caseywilliams force-pushed the PDS-442/breakpoints branch 2 times, most recently from 4c08df6 to da7edfa Compare February 13, 2020 22:02
@caseywilliams
Copy link
Contributor Author

(updated to fix some commit message typos)

Copy link
Contributor

@nkanderson nkanderson left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@vine77 vine77 left a comment

Choose a reason for hiding this comment

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

This is great @caseywilliams! One minor comment on the math. And can we add a little documentation about this feature? You could even stub out a responsive.md doc in packages/design-system-website/foundations if we want to have it on the website with a simple example like:

@include respond-below(medium) {
  width: 100%;
}

// Portrait-mode phones are generally smaller than the 'small' breakpoint.
'small': 576px,
// Landscape-mode phones are between 'small' and 'medium'.
'medium': 767px,
Copy link
Contributor

@vine77 vine77 Feb 20, 2020

Choose a reason for hiding this comment

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

This ends up being @media (max-width: 766px) instead of @media (max-width: 767px).

Suggested change
'medium': 767px,
'medium': 768px,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went ahead and just copied all the small, medium, and large values from bootstrap

// Landscape-mode phones are between 'small' and 'medium'.
'medium': 767px,
// Tablets + smaller desktop browser windows are between 'medium' and 'large'.
'large': 1179px,
Copy link
Contributor

Choose a reason for hiding this comment

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

That article you linked as well as component libraries like Bootstrap, Semantic UI, etc. use 992px as the "large" breakpoint.

Suggested change
'large': 1179px,
'large': 992px,

Were you thinking that we'd need something wider than that?

Copy link
Contributor Author

@caseywilliams caseywilliams Feb 21, 2020

Choose a reason for hiding this comment

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

Yeah, the max layout width on the new forge designs is quite large - I'll use the 992 value, though, and we can customize it on our end

Adds variables and mixins to the `sass-variables` package to allow for
configuring and responding to different breakpoints.

There are three breakpoints -- small, medium, and large -- which
correspond roughly to phone, tablet, and desktop widths.

Default breakpoints can be overridden by defining your own
`$breakpoints` map in your stylesheets before importing styles from the
design system.

Two mixins are included:

- `respond-above` applies styles to widths above the given size
- `respond-below` applies styles to widths below the given size
@caseywilliams
Copy link
Contributor Author

Ok I think this is ready for another review - I've included a page of docs for the site - totally open to all kinds of edits.

Copy link
Contributor

@vine77 vine77 left a comment

Choose a reason for hiding this comment

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

Thank you for the docs! Glad we’ll have this on the website. (Later on I may change the example just because we might want to encourage max-width, but we’ll see.) Will cut a release today.

@vine77 vine77 merged commit a2e36c0 into puppetlabs:master Feb 21, 2020
@vine77
Copy link
Contributor

vine77 commented Feb 21, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants