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

Allow OR operator for media queries #143

Closed
wants to merge 3 commits into from

Conversation

jackmcpickle
Copy link
Collaborator

@jackmcpickle jackmcpickle commented Oct 12, 2016

Ok its late but here is a WIP PR or addressing #102

Not the nicest solution, but a starting point - Doesn't really fit the guideline Simplicity is a keyword in include-media, try not to involve too much complexity. But open to suggestions and comments.

Use can see example gist here.

@jackmcpickle
Copy link
Collaborator Author

Input is

@include media('>phone', 'OR', '<tablet') {
  .red {
    color: red;
  }
}

@include media('>phone', 'OR', '<tablet', 'screen') {
  .red {
    color: red;
  }
}

Output

@media (min-width: 321px), (max-width: 767px) {
  .red {
    color: red;
  }
}

@media screen and (min-width: 321px), screen and (max-width: 767px) {
  .red {
    color: red;
  }
}

@nirazul
Copy link

nirazul commented Oct 12, 2016

Why does 'screen' belong to both conditions? I'd expect it to count towards '<tablet'.

Additonally, does your implementation support more than one OR condition? From looking at the changes, it doesn't seem so. But I'm not sure. I think that any implementation should support an arbitrary number of OR statements.

Generally, I still don't really like the or syntax. I'd prefer something like, but this decision is up to the authors:

@include media('>phone' 'screen', '<tablet' 'screen') {
    .red {
        color: red;
    }
}

@jackmcpickle
Copy link
Collaborator Author

@nirazul this is just how sass works by default. See this gist

any nested media query is added as an AND query.

Also while I agree that the syntax you suggest is better, this would be a big breaking change to include-media. And perhaps harder due to the nature of sass lists - both $list: one two; and $list: one, two; are lists.

@nirazul
Copy link

nirazul commented Oct 13, 2016

@jackmcpickle
Your gist shows nested media queries. I'm talking about this example:
Why is 'screen' applied to both as if it's a nested mediaquery?

@include media('>phone', 'OR', '<tablet', 'screen') { /* <-- this 'screen' is applied to both */
  .red {
    color: red;
  }
}

And yes, unfortunately it'd be a big breaking change :(
Maybe there's the option to add a new mixin media-or. To your concern with sass lists: This shouldn't be a concern as lists can't switch between separators back and forth. So one two, three fouris always a nested list construct that comprises one two and three four.

Here's an example output taken from this nice article: Lists in Sass

@jackmcpickle
Copy link
Collaborator Author

@nirazul screen is applied to both due to way include media processes the query list. Each item in the list is nested under the next except for now in the case of the OR.

But agreed maybe a media-or mixin is the right approach. Perhaps if @eduardoboucas and @hugogiraudel could share their vision on where the right approach should be.

@eduardoboucas
Copy link
Owner

Ideally, I'd want a syntax like:

@include media(('>medium', '<large'), ('>large', 'retina2x')) {}

Which targets anything between medium and large OR anything larger than large matching a retina2x expression. So effectively we'd be supplying multiple instances of the syntax we're already using.

I looked into this myself not too long ago and I parked it because I couldn't find a solution that wasn't ridiculously complex and backwards-compatible.

@nirazul
Copy link

nirazul commented Oct 14, 2016

Maybe I'll find some time this weekend to prepare a PR showing this behaviour.

@jackmcpickle
Copy link
Collaborator Author

Sounds good to me. @eduardoboucas feel free to close.

@eduardoboucas
Copy link
Owner

I'll leave it open for now. Your approach is perfectly valid, even if it ends up not being the final solution, and your effort is much appreciated.

Thanks!

@nirazul nirazul mentioned this pull request Oct 18, 2016
@eduardoboucas
Copy link
Owner

(See #144)

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

Successfully merging this pull request may close these issues.

3 participants