Support or conjunction

[nirazul]

This adds support for OR conditions as discussed in #143 and #102. Maintaining backwards compatibility with the old behaviour is done by checking the type of the first argument.

Old behaviour (arguments are strings):

@include media('landscape', '>phone') { /* stuff */ }

New behaviour:

@include media(('landscape' '>phone')) {} // landscape AND '>phone'
@include media(('landscape', '>phone')) {} // landscape OR '>phone'
@include media(('landscape' ('>phone', 'height>400px')) {} // landscape AND '>phone' OR landscape AND 'height>400px'

Usage:

• The first and only argument is a list
• AND conditions are described as lists with list-separator space
• OR conditions are described as lists with list-separator comma
• AND/OR lists can be nested at will and are resolved according to nesting hierarchy

There's only one gotcha in case the plugin user has added complex items to the $media-expressions list that have AND/OR conditions within the string, for example:

$media-expressions: (
    'exotic-query': '(min-width: 768px), (orientation: landscape)'
);

Such conditions need to be refactored into:

$media-expressions: (
    'exotic-query': ('(min-width: 768px)', '(orientation: landscape)')
);

The update turned out to be much more intrusive than I initially though. I'm well aware that this could be a reason to reject the PR.
In case the main approach is accepted: I think I've covered all possible cases but another pair of eyes testing and looking at the code is very much appreciated.

[KittyGiraudel]

I will take some time to review the implementation tomorrow. Thank you for doing this.

[eduardoboucas]

I will have a look too. Thanks!

[KittyGiraudel]

Alright. I had a quick look at the implementation, but that’s not what bothers me the most. I am relatively strongly against hooking logic on a list separator.

While I acknowledge that two identical lists with different separators are not equal, I still expect them to behave the same no matter where I use them. I don’t want to have to recreate a list with a different separator just for the sake of passing it in a specific mixin.

This will also lead to some linting issues with environments where a specific list separator is being enforced. That’s the case with anyone strictly following Sass Guidelines for instance, which expects commas as separators.

Of course I’ll let Eduardo decide on this matter, but I’d be against this. It has nothing to do with the implementation itself; the code looks pretty good and clean to me. Heck, there are even tests. Thi PR is fantastic. I just don’t believe in the rationale behind it.

If I am being entirely honest, I don’t see OR conditions making their way in include-media in its current state. The whole library rely on the fact that recursive @media directives in Sass result in AND conditions. The cleanest way I see to do it is effectively to allow lists as arguments, and not to go recursive on sublists to rely on the fact that Sass is clever enough to transform this:

.foo {
  @media (min-width: 400px), (orientation: landscape) {
    @media screen {
      color: red;
    }
  }
}

… into:

@media screen and (min-width: 400px), screen and (orientation: landscape) {
  .foo {
    color: red;
  }
}

The problem there is with that is handling a simple OR condition is getting hard. How do you syntactically make the difference between a OR b and a AND b? One would be an arglist, the other a list. In my opinion, that’s still better than checking the separator, but I could see bumps on the technical road.

Interesting discussion though. :)

[jackmcpickle]

Probably a good case for a plugin I reckon.

[eduardoboucas]

Apologies for the delayed reply.

I have to be honest: I personally never felt this to be an essential feature, but I was happy to see what we could come up with because lots of people seemed to be interested in it. Unfortunately, the added complexity this brings and the fact that it's not 100% backwards-compatible lead me to think it's not a good idea to make this part of the core library.

To be clear, this implementation is really good, and I'm sure it would be very useful to some people. @nirazul, if you're happy to maintain your fork of include-media, I'm more than happy to add a note to the README pointing to it any people interested in this feature.

I will close this PR, as well as #143, and I hope both @nirazul and @jackmcpickle understand the predicament here — it's nothing against your implementations, it's just that the risks involved outweigh the advantages.

Thanks all!

[nirazul]

Sure, no hard feelings about this. It's been fun to wrap my head around this problem.

Rather than a fork, I'd like to make a plugin out of this. Unfortunately I'm not sure how to do this or if it's even possible. In any case, if a plugin turns out not to be a feasible solution, I'm happy to create a fork with this implementation as I'm sure I'll use it in my projects.

@hugogiraudel @eduardoboucas
Do you have any ideas how to proceed with creating a plugin out of this PR?

[eduardoboucas]

@nirazul normally a plug-in would simply extend and/or override any of the variables, mixins or functions defined in the library. Traditionally they change smaller chunks of functionality than what you're looking to do, but I don't see that as being a major problem.

Shout if you need any more info.

[nirazul]

@eduardoboucas
I got some time to look into this. Unfortunately it's not easily possible for a plugin to overwrite media-expressions in a sensible way. So I went ahead with your first proposal and did a proper fork:

GitHub
npm package

[eduardoboucas]

Looks good! Added to README.

[nirazul]

Thank you! :)