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

Media query mixins #13014

Closed
wants to merge 17 commits into from
Closed

Media query mixins #13014

wants to merge 17 commits into from

Conversation

mrmrs
Copy link
Contributor

@mrmrs mrmrs commented Mar 11, 2014

This covers the majority of media queries present in bootstrap except for

  • print queries
  • queries that need the word 'screen' i.e carousel.less, jumbotron.less, and tables.less
  • anyplace where @grid-float-breakpoint is used to set the parameters of the query

Print queries will be super easy if we just add another mixin.
For the second and third cases - ideally you could override these defaults without having to add new mixins. Still looking into that.

@media (min-width: @screen-sm-min) {
// Automatically set modal's width for larger viewports
.screen-sm({
// Automatically set modal\'s width for larger viewports
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unnecessary backslash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cvrebert I made a comment about that in the commit - but maybe it's worth adding a comment in the file itself. That is needed to escape the ' which for some reason causes a parsing error in that comment once you use a mixin with a ruleset. Other option would be to have bad punctuation and just drop the ' altogether.

Copy link
Member

Choose a reason for hiding this comment

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

Let's report that upstream to the Less folks—we make extensive use of apostrophes 😁.

@cvrebert
Copy link
Collaborator

Nice, @mrmrs! I <3 refactoring.

We should probably squash this into fewer commits before merging it.

@cvrebert cvrebert added this to the v3.2.0 milestone Mar 11, 2014
@mrmrs
Copy link
Contributor Author

mrmrs commented Mar 11, 2014

@cvrebert Thanks :) I <3 it too

@cvrebert
Copy link
Collaborator

cvrebert commented May 4, 2014

So, did anyone find/report the Less bug upstream?

For the naming of the mixins, my suggestion would be screen-XX-only and screen-XX-plus.
@mdo Your thoughts on the mixin naming?

@cvrebert
Copy link
Collaborator

cvrebert commented May 4, 2014

Also, do we know whether this technique has a reasonable Sass translation (for bootstrap-sass)?

@cvrebert
Copy link
Collaborator

cvrebert commented May 4, 2014

Fixes #12974.

@cvrebert cvrebert mentioned this pull request May 6, 2014
@fubarhouse
Copy link

SASS appears to have a similar function.
I've been watching this for some time and I wanted it in one of my projects for work.
I haven't quite tested it, but see below for the mixins i've writted specific to my project and the explanation of the syntax.

@mixin screen-sm() {
    @media (max-width: 812px) {
        @content;
    }
}
@mixin screen-md() {
    @media (min-width: 813px) and (max-width: 1076px) {
        @content;
    }
}
@mixin screen-lg() {
    @media (min-width: 1077px) {
        @content
    }
}

Source:
http://mikefowler.me/2011/12/08/passing-content-to-mixins-in-sass/

I've got to do some more testing but it should do the trick if you change to bootstraps spec.

Edit: I've just done some testing, works like a charm using the following declarations:

.no-display-small {
    @include screen-sm() {
        display:none;
    }
}
@include screen-sm() {
    .no-display-small {
        display:none;
    }
}

Edit: I've just finalised it in bootstrap, and converted the existing LESS file to SASS.
You can see it here: https://github.com/fubarhouse/easyflex/blob/master/themes/easyflex/sass/global/bootstrap-media-queries.scss

@bassjobsen
Copy link
Contributor

Please also see: #13619

I think your solution can work, personally i wonder if you should wrap media queries in a mixin this way. Media queries already have their own logical operators and preceding rules. Using this media queries mixins feels less intuitive for me now. If you have to combine two media queries for some reason, you will get:

.screen-xs({.screen-retina({color:red;})});
vs
@media (max-width: @screen-xs-max) and ( @retina-displays ){color:red;}

But when testing this mixin strategy there seems noting wrong with the out put. I found:

@screen-xs-max:  480px-1;

// xs only
.screen-xs(@rules) {
    @media (max-width: @screen-xs-max) { @rules(); }
}

// retina display
.screen-retina(@rules) {

  @media
  only screen and (-webkit-min-device-pixel-ratio: 2),
  only screen and (   min--moz-device-pixel-ratio: 2),
  only screen and (     -o-min-device-pixel-ratio: 2/1),
  only screen and (        min-device-pixel-ratio: 2),
  only screen and (                min-resolution: 192dpi),
  only screen and (                min-resolution: 2dppx)
     { @rules(); }
}

.test {
.screen-xs({.screen-retina({color:red;})});
}

will compile into:

@media (max-width: 479px) and only screen and (-webkit-min-device-pixel-ratio: 2), (max-width: 479px) and only screen and (min--moz-device-pixel-ratio: 2), (max-width: 479px) and only screen and (-o-min-device-pixel-ratio: 2/1), (max-width: 479px) and only screen and (min-device-pixel-ratio: 2), (max-width: 479px) and only screen and (min-resolution: 192dpi), (max-width: 479px) and only screen and (min-resolution: 2dppx) {
  .test {
    color: red;
  }
}

The output of @media (max-width: @screen-xs-max) and ( @retina-displays ){color:red;} seems shorter:

@media (max-width: 479px) and (only screen and (-webkit-min-device-pixel-ratio: 2), only screen and (min--moz-device-pixel-ratio: 2), only screen and (-o-min-device-pixel-ratio: 2/1),  only screen and (min-device-pixel-ratio: 2),  only screen and (min-resolution: 192dpi),only screen and (min-resolution: 2dppx)) {
  .test2 {
    color: red;
  }
}

@fubarhouse
Copy link

You could well add another retina mixin, to apply to particular elements such as an image.

@mixin screen-retina() {
    @media only screen and (-webkit-min-device-pixel-ratio: 2) {
        @content
    }
}

or for more flexibility...

@mixin screen-retina($pixel-ratio: 2) {
    @media only screen and (-webkit-min-device-pixel-ratio: $pixel-ratio) {
        @content
    }
}

img.ratio-1 { @include screen-retina($pixel-ratio: 1); }
img.ratio-2 { @include screen-retina(); }

Retina displays and retina CSS is in fact trending and it would be wise to include the ability to do this out of the box. I would just create an extra less/scss file to include custom mixins but for the greater good we will eventually need this.

@fubarhouse
Copy link

I've duplicated the changes in this issue log in bootstrap sass and requested a pull.
twbs/bootstrap-sass#617

@cvrebert
Copy link
Collaborator

@fubarhouse I believe the Sass port is mostly autogenerated, so they might need a patch to their Less=>SCSS conversion script instead of a patch to the SCSS directly.

@cvrebert cvrebert modified the milestones: v3.3.0, v3.2.1 Jun 9, 2014
@mdo
Copy link
Member

mdo commented Jul 6, 2014

Punting to v4 per the issue.

@mdo mdo closed this Jul 6, 2014
@mdo mdo removed this from the v3.3.0 milestone Jul 6, 2014
@mdo mdo deleted the media-query-mixins branch October 27, 2014 06:16
@gotbahn gotbahn mentioned this pull request Apr 23, 2015
@mdo mdo mentioned this pull request Aug 19, 2015
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.

6 participants