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

Unitless breakpoints #27190

Merged
merged 5 commits into from
Oct 21, 2018
Merged

Unitless breakpoints #27190

merged 5 commits into from
Oct 21, 2018

Conversation

johanlef
Copy link
Contributor

@johanlef johanlef commented Sep 7, 2018

If I want to customise the breakpoints using em, I get compatibility errors.

$grid-breakpoints: (
    default: 0em,
    watch: 20em,
    phone: 30em
    // etc.
}

It is good practice to set breakpoints in em instead of px when users change font size in their browser. See https://zellwk.com/blog/media-query-units/#concluding-the-experiments for more information why someone would like to do this. Only Safari users could get annoyed: https://adamwathan.me/dont-use-em-for-media-queries/

In any case, using a unitless number at line 42 would be very convenient.

If I want to customise the breakpoints using `em`, I get compatibility errors.

It is good practice to set breakpoints in `em` instead of `px` when users use browser scaling.
See https://zellwk.com/blog/media-query-units/#concluding-the-experiments for more information why someone would like to do this. Only Safari users can get annoyed: https://adamwathan.me/dont-use-em-for-media-queries/

In any case, using a unitless number at line 42 would be very convenient.
@browner12
Copy link
Contributor

While it seems like this may be a bad option according to Adam's article, it makes sense to me to make this change, and allow the user to choose whether or not the Safari bug is an issue for them.

I was not familiar with SCSS math, so I did a little digging into whether this is valid, and it turns out, it works! If you use a unitless value with a value with a unit, the unit is preserved. However, mixing units will result in an error.

.style {
    height: 500em - 100;
    width: 500px - 100;
}

compiles to

.style {
    height: 400em;
    width: 400px;
}

Copy link
Collaborator

@andresgalante andresgalante left a comment

Choose a reason for hiding this comment

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

Hi @johanlef thanks a lot for your contribution.

There has been a lot of discussion around this issue, among them #22006, #25632, #19619, #25166, and others related to why we are using pxs instead of rems or ems

Personally, I think we should look into moving to relative units, but at this point, it'd be a breaking change for v4.

Having said that I think this is a good discussion point for v5.

@johanlef
Copy link
Contributor Author

Okay, I understand moving to other units will be a breaking change.

@andresgalante Going from px to unitless on this particular line is not a breaking change: as @browner12 pointed out, the units are preserved.

@browner12
Copy link
Contributor

Correct me if I'm wrong, but I don't believe this would be a breaking change in v4.

As I showed in my first comment, when doing math in SCSS, if one of the values is unitless, the other units will be preserved.

By merging this PR, all existing code would work as is, and it would allow people who choose to use em breakpoints to not run into a compile error.

@johanlef
Copy link
Contributor Author

@andresgalante My proposal is about removing the unit, not changing to another one. When the unit is removed, the original unit is preserved, so this change would not cause any issues.

Copy link
Member

@mdo mdo left a comment

Choose a reason for hiding this comment

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

I think this is fine? Not sold on em-based media queries yet as we've gone back and forth on that a lot, but this seems fine.

@XhmikosR
Copy link
Member

Just so that we are safe, is this backwards compatible?

@mdo
Copy link
Member

mdo commented Oct 21, 2018

@XhmikosR I believe so—this is Sass math, and previously we required folks to use pixels only for media queries. This opens it up so others can use ems, and doesn't prevent folks from using pixels.

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

Successfully merging this pull request may close these issues.

6 participants