Skip to content
New issue

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

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

Already on GitHub? # to your account

HiDPI breakpoints #9661

Merged
merged 11 commits into from
Jul 24, 2019
Merged

Conversation

ncoden
Copy link
Contributor

@ncoden ncoden commented Jan 17, 2017

Closes Partially fix #9556.

Add hidpi-1-5, hidpi-2 and hidpi-3 breakpoints.

Usage

@include breakpoint(hidpi-2) {
  // HiDPI x2 and up.
}

@include breakpoint(hidpi-2 only) {
  // Between HiDPI x2 and  x3 (excluded).
}

@include breakpoint(hidpi-2 down) {
  // HiDPI x3 (excluded) and down.
}

Any HiDPI breakpoint can be added in the $breakpoints-hidpi map (the order does not matter):

$breakpoints-hidpi: (
  hidpi-1: 1,
  hidpi-1-5: 1.5,
  hidpi-2: 2,
  retina: 2,
  hidpi-3: 3
) !default;

Changes:

  • Refactor breakpoint function.
  • Add $breakpoints-hidpi map: A list of named HiDPI breakpoints with HiDPI ratios (without units).
  • Add support for HiDPI breakpoints (up, only, down) in @function breakpoint.
  • Add unit tests for HiDPI breakpoint with default (up), up, only and down ranges.

Other changes:

  • Add @function -zf-map-next-number: Find the next number in a map.
  • Add @function -zf-bp-join: Return media query string from the given min and/or
    max limits.
  • Add @function zf-str-join: Return a join of the two given strings $str1 and
    $str2. If the two strings are not empty, they are separated by
    $delimiter.

No breaking changes: HiDPI breakpoints works exactly like standard breakpoints. retina is an alias for hidpi-2.

Changes:
- Factorize breakpoint value check and min/max limits computation.
- Factorize breakpoint string generation.
Changes:
- Add `$breakpoints-hidpi` map: A list of named HiDPI breakpoints with
HiDPI ratios (without units).
- Add support for HiDPI breakpoints (up, only, down) in `@function
breakpoint`.

Other changes:
- Add `@function -zf-map-next-number`: Find the next number in a map.

No breaking changes: HiDPI breakpoints works exactly like standard
breakpoints. `retina` is an alias for `hidpi-2`.
Add unit tests for HiDPI breakpoint with default (up), up, only and
down ranges.
Changes:
- Add separation comments
- Change some test descriptions
Media Queries does not accept nested logic.

Changes:
- Generate a valid media query using string joining functions.
- Update unit tests

Add required functions:
- `-zf-bp-join`: Return media query string from the given min and/or
max limits.
- `zf-str-join`: Return a join of the two given strings `$str1` and
`$str2`. If the two strings are not empty, they are separated by
`$delimiter`.
@ncoden
Copy link
Contributor Author

ncoden commented Jan 20, 2017

So, some news: I looked at how to add HiDPI breakpoints to Interchange. It will require a refactor of Interchange and util.MediaQuery since these two files are built on the assumption there is only size breakpoint and only one breakpoint is valid at the same time.

It would also be good to pass datas from SCSS to Js properly throw JSON instead of serialised map. But I do not want to add any SCSS library (SassyJSON) before having a proper SCSS dependency management. So it will wait for #9542.

@kball @nlap Let me know it you want the Interchange support to be added in this PR or an other.

@nlap
Copy link

nlap commented Jan 23, 2017

Hmm I see, I hadn't looked into it enough until now to notice the interchange retina query wasn't passed from the SCSS.

FWIW my use for #9556 was specifically with Interchange, but I think this PR adds value as it is.

@kball
Copy link
Contributor

kball commented Jan 23, 2017

I concur, lets keep the interchange refactor separate. @brettsmason @andycochran as our other "sassy" yetinauts, can one of you review? ;)

@kball
Copy link
Contributor

kball commented Apr 18, 2017

ping @andycochran can you take a look at this?

@kball kball requested a review from andycochran April 18, 2017 22:31
@andycochran andycochran self-assigned this Apr 19, 2017
@ncoden ncoden force-pushed the feat/hidpi-breakpoints-9556 branch from 37cef7f to c3dc69d Compare December 23, 2017 13:54
I did not read correctly the W3C documentation.
* DPI = dots per inch = device pixel per inch
* DPPX = dots per pixel
* 1in = 72pt = 96px

So:
* 1dpi = 1/72dppt = 1/96dppx
* 1dppx = 72dppt = 96dpi
@ncoden
Copy link
Contributor Author

ncoden commented Dec 24, 2017

Note: this PR only add support for hidpi in the breakpoint mixin. A refactor of the way breakpoints are handled across the whole framework is required to add support in visibility classes, grids & cie.

The problem mainly comes from that functions/mixins will manually get the $breakpoints map instead of relying on the breakpoint mixins. This is not hard to refactor, but it takes some time to be sure nothing has been broken.

@DanielRuf
Copy link
Contributor

Note: this PR only add support for hidpi in the breakpoint mixin. A refactor of the way breakpoints are handled across the whole framework is required to add support in visibility classes, grids & cie.

The problem mainly comes from that functions/mixins will manually get the $breakpoints map instead of relying on the breakpoint mixins. This is not hard to refactor, but it takes some time to be sure nothing has been broken.

Which is still relevant also for #10979

@nlap
Copy link

nlap commented Dec 21, 2018

Bumping this per discussion in #9556

Although my main use case was with Interchange, I think this PR is a great addition, makes more sense than the current conventions, and should be pulled in 👍

@ncoden ncoden added this to the 6.6.0 milestone Dec 23, 2018
Copy link
Contributor

@kball kball left a comment

Choose a reason for hiding this comment

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

Approved, fixing merge conflicts and merging.

@kball kball merged commit 92c9f2e into foundation:develop Jul 24, 2019
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

retina named query
5 participants