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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add narrow/regular/wide viewport range utilities #1995

Merged
merged 7 commits into from Mar 25, 2022

Conversation

vdepizzol
Copy link
Contributor

What are you trying to accomplish?

This PR adds viewport range utilities alongside our visibility/display options. It builds on the new Narrow/Regular/Wide nomenclature we're starting to adopt, which will be needed for new components such as PageHeader.

This proposal has been described as part of https://github.com/github/primer/issues/580 (only available for hubbers).

Are additional changes needed?

  • No, this PR should be ok to ship as is. 馃殺

@vdepizzol vdepizzol requested a review from a team as a code owner March 23, 2022 22:28
@changeset-bot
Copy link

changeset-bot bot commented Mar 23, 2022

馃 Changeset detected

Latest commit: 6314d8d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vdepizzol vdepizzol requested a review from simurai March 23, 2022 22:43
Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

Looks good!

}
}

@media (min-width: $width-md) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Something I'm wondering... should this be limited to

Suggested change
@media (min-width: $width-md) {
@media (min-width: $width-md) and (max-width: $width-xl - 0.02px) {

so that these utilities only kick in when between the $width-md and $width-xl breakpoint and not from $width-md upwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simurai That's in fact how we're considering our viewport ranges to go. Take a look at https://github.com/github/primer/issues/580#issuecomment-1076890831 to see the media queries we're proposing. You can also see a visual of these breakpoint/viewport considerations here in this Figma frame. (only available for hubbers) 馃榿

The idea is for Narrow to wrap all "mobile"-friendly design patterns, and Regular all the "desktop"-friendly ones, including wider scenarios. The Wide viewport range in that case is a subset of Regular, applied on top of it.

鉁岋笍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simurai your comment made me thing of a scenario I haven't considered: when some area needs visibility on Regular but not on Wide scenarios.

I think we can support it like this:

<div class="show-whenRegular hide-whenWide">
  ...
</div>

I've played with CSS specificities (in a way that doesn't try to reinvent the wheel in regards to the use of !important in our Primer CSS utilities) in this prototype, and have updated the PR accordingly.

Thank you! 馃檱

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Narrow is a range (0 - 768), but Regular and Wide behave more like "breakpoints" (from ... to infinity).

Ok, yeah.. that is probably safer and you don't have to remember to add both Regular and Wide. 馃憤

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

Successfully merging this pull request may close these issues.

None yet

4 participants