fix: add the print media query for the right media query combinations #10979

DanielRuf · 2018-02-22T23:31:24Z

This covers more down, up and only cases.

Reference: #10976

ncoden · 2018-02-22T23:38:40Z

scss/util/_breakpoint.scss

@@ -166,7 +168,7 @@ $breakpoint-classes: (small medium large) !default;
    // Otherwise, wrap the content in a media query
    @else {
      // For named breakpoints less than or equal to $print-breakpoint, add print to the media types
-      @if $bp != null and $bp <= $pbp {
+      @if ($bp != null and $bp == $pbp and ($dir == '' or $dir == 'up' or $dir == 'only')) or ($bp != null and $bp < $pbp and ($dir == '' or $dir == 'up'))  or ($bp != null and $bp >= $pbp and ($dir == 'down')) {


I think we should move this to a cleaner "breakpoint compairaison" function. Putting all the cases here in a if is really confusing.

Definitely. Could need some assistance here as I've not often written mixins and functions.

I'll take care of this.

... this weekend :)

@DanielRuf @ncoden I'm not sure if this line has to be changed at all... what is the goal?

The comment says 'for named breakpoints less than or equal to $print-breakpoint' what I interpreted as starting point . The original @include breakpoint(large) was actually a @include breakpoint(large up) and thus also affects breakpoints greater than large.

So is the goal that breakpoint(large only) and breakpoint(large down) do affect print but breakpoint(large) does not? (although this would be a breaking change)
If not the condition $bp != null and $bp <= $pbp doesn't need to be changed imo

I had some trouble to retrieve the problem here and the purpose of this change as we are working with confusing intervals here, but I think I got it.

If I understand $print-breakpoint correctly, it must be used to prevent print to consider highest breakpoints because, despite the high resolution of the paper (which is good for reading), this is not a good support for small interfaces and interactive elements. Paper should almost be considered as a tablet giving a fake resolution (like iPads, but we are faking the resolution).

Following this, the objective is to make printable the styles applied on $print-breakpoint or below like if higher breakpoints did not exists. This means that, for $print-breakpoint: large;:

large up/only/down, medium up/only/down, xlarge down should be printable because their styles are applied to a large (or smaller) screen so we should consider them as displayable on a large (or smaller) print support.

xlarge only/up should be not printable like before.

@SassNinja What do you think ?

Ok @ncoden so the rule would be to generate print if the breakpoint affects the print-breakpoint (or smaller).
This means the current condition only needs to be extended so 'down' always generates print.

Since @DanielRuf has already added $dir I would simply change the condition to

@if $bp != null and ($bp <= $pbp or $dir == down) {

Yes, and we can add an explantation comment and a link to the comment here. @DanielRuf What do you think ?

Sounds good. I might have thought to complex. Not sure what I covered that would not be covered by the simple or condition. 🤔

ncoden · 2018-02-22T23:43:19Z

Also, there is a lot of cleaning to do in the breakpoint function and mixin. See https://github.com/zurb/foundation-sites/pull/9661/files ;)

scss/util/_breakpoint.scss

DanielRuf · 2018-03-17T21:37:09Z

Also, there is a lot of cleaning to do in the breakpoint function and mixin. See https://github.com/zurb/foundation-sites/pull/9661/files ;)

Do we need any further changes in this PR here as we also need #9661?

ncoden · 2018-03-17T21:43:42Z

Do we need any further changes in this PR here as we also need #9661?

See #10979 (comment)

DanielRuf · 2018-03-17T22:08:18Z

But doesn't this exclude / miss some cases?

Think about print breakpoint large.
I have an element which uses medium down. But this would be printed with the code of the comment I guess. But it should not because it should include $bp >= $pbp and ($dir == 'down')

DanielRuf · 2018-03-17T22:09:25Z

Think about print breakpoint large.
I have an element which uses medium down. But this would be printed with the code of the comment I guess. But it should not because it should include $bp >= $pbp and ($dir == 'down')

/cc @SassNinja

ncoden · 2018-03-17T22:12:29Z

@DanielRuf
I'm not sure to understand the case. Do you mean an element using breakpoint(medium down) with $print-breakpoint: large ? If so, "print" will be in the generated breakpoint because medium < large.

DanielRuf · 2018-03-17T22:25:44Z

But the element will not be visible on large so there should not be a print media query.

@media print, screen and (max-width: medium) { would not be right then.

ncoden · 2018-03-17T23:02:15Z

hmm... There is a problem with the current implementation:

@media print, screen and (max-width: medium) {

...means "print OR (screen and max-width: medium)".

So for the following code:

$print-breakpoint: medium;

.only-for-small,
.only-for-medium {
    display: none;
}

@include breakpoint(small only) {
    .only-for-small { display: block }
}

@include breakpoint(medium only) {
    .only-for-medium { display: block }
}

<div class="only-for-small">SMALL</div>
<div class="only-for-medium">MEDIUM</div>

Both SMALL and MEDIUM would be visible on print... I don't think this is expected.
See: https://codepen.io/ncoden/pen/RMGOdx

So we may have to change that to https://codepen.io/ncoden/pen/BrLEeO OR https://codepen.io/ncoden/pen/wmzZLQ

DanielRuf · 2018-03-22T21:28:43Z

Both SMALL and MEDIUM would be visible on print... I don't think this is expected.
See: https://codepen.io/ncoden/pen/RMGOdx

Huh? I don't think so. At least with my proposed version it should be on only and down afaik.

DanielRuf · 2018-03-22T21:29:31Z

https://github.com/zurb/foundation-sites/pull/10979/files#diff-bf1c44b23206fb646211490bd0d02fffR171

ncoden · 2018-03-22T22:06:03Z

Huh? I don't think so. At least with my proposed version it should be on only and down afaik.

Your proposed version does not resolve the case but hide some of its effects. The bug has nothing to do with "when do we add print" but is caused by the way we generate the mediaquery, with "print OR [breakpoint]".

DanielRuf · 2018-03-22T22:13:20Z

Ah okay.

DanielRuf · 2018-07-01T12:37:42Z

What are the next steps here?

ncoden · 2018-07-01T16:55:43Z

I took a look at the whole discussion and I think that the todo-list is:

Add support for down breakpoints https://github.com/zurb/foundation-sites/pull/10979/files#r171845415
Ensure that only the right breakpoint is shown on print fix: add the print media query for the right media query combinations #10979 (comment)

But it's not a priority before v6.5.0 stable version is released.

kball · 2019-08-12T21:46:33Z

@DanielRuf do you have a sense of current status of this PR?

DanielRuf · 2019-08-12T22:03:29Z

@DanielRuf do you have a sense of current status of this PR?

I'll check it this week.

DanielRuf · 2019-08-15T20:30:07Z

@DanielRuf do you have a sense of current status of this PR?

Probably just needs the proposed change from https://github.com/zurb/foundation-sites/pull/10979/files#r171845415

generate print if the breakpoint affects the print-breakpoint (or smaller). This means the current condition only needs to be extended so 'down' always generates print.

joeworkman

LGTM

joeworkman · 2019-12-06T19:24:43Z

This pull request has been mentioned on Foundation Open Source Community. There might be relevant details there:

https://foundation.discourse.group/t/foundation-for-sites-v6-6-0-is-here-farout/30/1

fix: add the print media query for the right media query combinations

36a199f

DanielRuf requested a review from SassNinja February 22, 2018 23:32

ncoden reviewed Feb 22, 2018

View reviewed changes

ncoden added Sass Mixins bug fix labels Feb 22, 2018

rossta reviewed Feb 23, 2018

View reviewed changes

scss/util/_breakpoint.scss Show resolved Hide resolved

DanielRuf mentioned this pull request Feb 24, 2018

HiDPI breakpoints #9661

Merged

ncoden self-assigned this Feb 26, 2018

ncoden added this to the 6.5.0 milestone Mar 2, 2018

ncoden added the 💥 migration needed label Mar 2, 2018

Simplified breakpoint print if logic as agree to

a3a6262

generate print if the breakpoint affects the print-breakpoint (or smaller). This means the current condition only needs to be extended so 'down' always generates print.

joeworkman self-requested a review December 5, 2019 22:17

joeworkman reviewed Dec 5, 2019

View reviewed changes

joeworkman self-requested a review December 5, 2019 22:25

joeworkman approved these changes Dec 5, 2019

View reviewed changes

joeworkman merged commit 30ede00 into foundation:develop Dec 5, 2019

weekly-digest bot mentioned this pull request Dec 8, 2019

Weekly Digest (1 December, 2019 - 8 December, 2019) #11927

Closed

DanielRuf mentioned this pull request Jan 10, 2020

.hide-for-small-only and related visibility classes always hide for print media #11961

Closed

DanielRuf deleted the fix/print-media-query-calculation branch January 10, 2020 17:28

This was referenced Mar 10, 2020

[Snyk] Upgrade foundation-sites from 6.5.3 to 6.6.1 BarkleyREI/brei-project-scaffold#27

Closed

[Snyk] Upgrade foundation-sites from 6.5.3 to 6.6.1 BarkleyREI/brei-sass-boilerplate#81

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add the print media query for the right media query combinations #10979

fix: add the print media query for the right media query combinations #10979

DanielRuf commented Feb 22, 2018

ncoden Feb 22, 2018

DanielRuf Feb 22, 2018

ncoden Feb 24, 2018

DanielRuf Feb 28, 2018

ncoden Feb 28, 2018

SassNinja Mar 1, 2018

ncoden Mar 1, 2018 •

edited

Loading

SassNinja Mar 2, 2018

ncoden Mar 2, 2018 •

edited

Loading

DanielRuf Mar 2, 2018

ncoden commented Feb 22, 2018

DanielRuf commented Mar 17, 2018

ncoden commented Mar 17, 2018

DanielRuf commented Mar 17, 2018 •

edited

Loading

DanielRuf commented Mar 17, 2018

ncoden commented Mar 17, 2018

DanielRuf commented Mar 17, 2018 •

edited

Loading

ncoden commented Mar 17, 2018 •

edited

Loading

DanielRuf commented Mar 22, 2018

DanielRuf commented Mar 22, 2018

ncoden commented Mar 22, 2018

DanielRuf commented Mar 22, 2018

DanielRuf commented Jul 1, 2018

ncoden commented Jul 1, 2018 •

edited

Loading

kball commented Aug 12, 2019

DanielRuf commented Aug 12, 2019

DanielRuf commented Aug 15, 2019

joeworkman left a comment

joeworkman commented Dec 6, 2019

fix: add the print media query for the right media query combinations #10979

fix: add the print media query for the right media query combinations #10979

Conversation

DanielRuf commented Feb 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoden Mar 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoden Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoden commented Feb 22, 2018

DanielRuf commented Mar 17, 2018

ncoden commented Mar 17, 2018

DanielRuf commented Mar 17, 2018 • edited Loading

DanielRuf commented Mar 17, 2018

ncoden commented Mar 17, 2018

DanielRuf commented Mar 17, 2018 • edited Loading

ncoden commented Mar 17, 2018 • edited Loading

DanielRuf commented Mar 22, 2018

DanielRuf commented Mar 22, 2018

ncoden commented Mar 22, 2018

DanielRuf commented Mar 22, 2018

DanielRuf commented Jul 1, 2018

ncoden commented Jul 1, 2018 • edited Loading

kball commented Aug 12, 2019

DanielRuf commented Aug 12, 2019

DanielRuf commented Aug 15, 2019

joeworkman left a comment

Choose a reason for hiding this comment

joeworkman commented Dec 6, 2019

ncoden Mar 1, 2018 •

edited

Loading

ncoden Mar 2, 2018 •

edited

Loading

DanielRuf commented Mar 17, 2018 •

edited

Loading

DanielRuf commented Mar 17, 2018 •

edited

Loading

ncoden commented Mar 17, 2018 •

edited

Loading

ncoden commented Jul 1, 2018 •

edited

Loading