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

Extend media queries #1333

Closed
jonschlinkert opened this issue May 19, 2013 · 14 comments
Closed

Extend media queries #1333

jonschlinkert opened this issue May 19, 2013 · 14 comments

Comments

@jonschlinkert
Copy link
Contributor

Differs from "extend around media queries", #1165.

This is a feature request for extending media queries themselves, which should help make media queries less challenging to manage, and when used in conjunction with extending mixins could potentially resolve other standing Issues with media queries (such as those proposed in #1165).

I'm not sure about the syntax, I'd like to see other ideas but here are a couple of obvious ones to throw into the hat:

@media:extend only screen and (min-device-pixel-ratio: 1.5) {
    ...
}

// or
@extend:media only screen and (min-device-pixel-ratio: 1.5) {
    ...
}

Expected output would follow the same conventions as other extended expressions in Less.

@matthew-dean
Copy link
Member

Idea is good. Syntax is...I don't know...

I feel like maybe we should create media mixins. That's just a lot of mess for one line.

I think this is a good use case for that other issue raised of inserting content into one mixin output. Maybe some kind of special mixin class that allows wrapping of content, and sending it to the mixin as a @content var.

So, more like (this is not recommended syntax, just playing with the idea):

@group media-mixin() {
  @media only screen and (min-device-pixel-ratio: 1.5) {
    @content;
  }
}
@media-mixin() {
  .a { color: red; }
  .b { color: blue; }
}
@media-mixin() {
  .c { color: yellow; }
}

// Outputs
  @media only screen and (min-device-pixel-ratio: 1.5) {
    .a { color: red; }
    .b { color: blue; }
    .c { color: yellow; }
  }

In addition, it could support something like this (grouping by value):

@group media-mixin(@query) {
  @media @query {
    @content;
  }
}
@media-mixin(~"only screen and (min-device-pixel-ratio: 1.5)") {
  .a { color: red; }
  .b { color: blue; }
}
@media-mixin(~"only screen and (min-device-pixel-ratio: 1.5)") {
  .c { color: yellow; }
}
@media-mixin(~"only screen and (min-device-pixel-ratio: 2)") {
  .d { color: black; }
}


// Outputs
  @media only screen and (min-device-pixel-ratio: 1.5) {
    .a { color: red; }
    .b { color: blue; }
    .c { color: yellow; }
  }
  @media only screen and (min-device-pixel-ratio: 2) {
    .d { color: black; }
  }

@matthew-dean
Copy link
Member

I'm referring to something like Issue #965, although the syntax suggested there still looks strange to me.

@jonschlinkert
Copy link
Contributor Author

@MatthewDL, note that I said:

I'm not sure about the syntax

I'm sure "media query mixins" would be useful to some, but I personally wouldn't use them if media queries could be extended. I think the interpolation is another issue.

@matthew-dean
Copy link
Member

Well, it could be a case of two birds, one stone. If we could define a special mixin container that passes in a content block, as requested in #965, then we wouldn't risk muddying the extend syntax. It doesn't seem to make sense here, since :extend is in the format in a pseudo-selector, meaning it is designed to be attached to a selector, such as in the default extend syntax.

A media query is not a selector, so it is a head scratcher. As an alternative of the same idea, we could have a media block that is "self-grouping" without using an extend pseudo-selector, such as:

@media-group only screen and (min-device-pixel-ratio: 1.5) {
    ...
}

or, if you like:

@media-extend only screen and (min-device-pixel-ratio: 1.5) {
    ...
}

I just wouldn't write it as an pseudo-selector when it's not intermingled with selectors. @media:extend doesn't follow the form of selector syntax, nor the @ syntax.

@matthew-dean
Copy link
Member

But, similar to feedback some have had about mixins, using the keyword "extend" when it's not "extending" a specific class (selector) feels like incorrect usage as well. So, I would suggest that we resist the urge to keep piling things under the word "extend", especially when that behavior is not consistent with the way extend is used / defined for selectors.

Again, I'm not saying the specific word "group" is better or a suggested replacement. It's just that the extend syntax that was settled on was complicated enough and had such widely varied expectations to begin with.

AND since there's already a current request for @content mixins, with other real-world uses, we could solve the same problem with a syntax for both.

@jonschlinkert
Copy link
Contributor Author

Actually, on second thought since a feature request for extending mixins already exists, #1177, and media queries can be grouped in mixins, I'm closing this to put more support behind extending mixins. If that happens, many problems are solved.

@matthew-dean
Copy link
Member

How would you solve this in an extended mixin? Unless we can "wrap" content?

@jonschlinkert
Copy link
Contributor Author

We can do this currently:

.one {
    @media (width: 400px) {
        font-size: 1.2em;
        @media print and color {
            color: blue;
        }
    }
}
.two:extend(.one) {}

Outputs:

@media (width: 400px) {
  .one,
  .two {
    font-size: 1.2em;
  }
}
@media (width: 400px) and print and color {
  .one,
  .two {
    color: blue;
  }
}

I was thinking of .one as an implicit mixin, since that's the way I would use it, but really it's just a class. To your point, current functionality wouldn't allow you to actually extend the styles inside the media queries using this method. But my thinking is that one of the other standing media query issues might resolve that, in which case extending media queries might end up being a redundant feature. Am I assuming too much? Maybe I should re-review the other media query issues...

@matthew-dean
Copy link
Member

Yeah, the way you're using it is one valid use case, but it doesn't cover what I was talking about, which is what I thought you were saying: to literally to extend the media query. In other words, to dump content from different blocks into the same media query.

This does not currently collapse:

.one {
  color: white;
  @media print {
    color: yellow;
  }
  border-color: black;
  @media print {
    border-color: green;
  }
}

Produces:

.one {
  color: white;
  border-color: black;
}
@media print {
  .one {
    color: yellow;
  }
}
@media print {
  .one {
    border-color: green;
  }
}

(Interestingly, the above of course groups the properties for .one, changing the declaration order.)

So, extending likewise does not group media queries, since LESS never auto-collapses any kind of block because of the same cascade concerns.

.one {
  color: white;
  @media print {
    color: yellow;
  }
  border-color: black;
  @media print {
    border-color: green;
  }
}
.two:extend(.one) {
  @media print {
    color: red;
  }
}

Produces

.one,
.two {
  color: white;
  border-color: black;
}
@media print {
  .one,
  .two {
    color: yellow;
  }
}
@media print {
  .one,
  .two {
    border-color: green;
  }
}
@media print {
  .two {
    color: red;
  }
}

So, this issue should be reopened. Current extend syntax does not allow media query extending / grouping.

@jonschlinkert
Copy link
Contributor Author

fair enough, you've kept up on the other media query issues more than I, you don't think any of the other ones cover the mixin side of it?

@jonschlinkert jonschlinkert reopened this May 24, 2013
@matthew-dean
Copy link
Member

Yes, I think issue #965 instead of #1177 has proposals that cover this. I didn't really get the concept of it at first, but

  1. if you can extend mixins, and
  2. those mixins can accept selector blocks, and
  3. that content can be inserted into a @content placeholder,

then, by default, you SHOULD be able to group media queries just by inserting those into a @content mixin.

So, yeah, this could be closed in favor of #965, if that issue can be implemented correctly. The proposed syntax thus far needs some work, so you should go read through and apply your syntax brain.

@matthew-dean
Copy link
Member

Closed to move discussion into #965.

@jonschlinkert
Copy link
Contributor Author

👍 sounds good

@matthew-dean
Copy link
Member

Note: this seems to be is a duplicate of Issue #950.

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

No branches or pull requests

2 participants