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.

Sign up for GitHub

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

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

Merged
merged 2 commits into from Dec 5, 2019
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
36a199f
fix: add the print media query for the right media query combinations
DanielRuf Feb 22, 2018
a3a6262
Simplified breakpoint print if logic as agree to
joeworkman Dec 5, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions scss/util/_breakpoint.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,10 @@ $breakpoint-classes: (small medium large) !default;
@for $i from 1 through length($values) {
$value: nth($values, $i);
$str: breakpoint($value);
$bp: index($-zf-breakpoints-keys, $value);
$bp: index($-zf-breakpoints-keys, nth($value, 1));
joeworkman marked this conversation as resolved.
Show resolved Hide resolved
$pbp: index($-zf-breakpoints-keys, $print-breakpoint);
// Direction of media query (up, down, or only)
$dir: if(length($value) > 1, nth($value, 2), up);

$old-zf-size: null;

Expand All @@ -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')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll take care of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks =)

Copy link
Contributor

Choose a reason for hiding this comment

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

... this weekend :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@ncoden ncoden Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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) {

Copy link
Contributor

@ncoden ncoden Mar 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@media print, screen and #{$str} {
@content;
}
Expand Down