Code Monkey home page Code Monkey logo

Comments (24)

eduardoboucas avatar eduardoboucas commented on May 18, 2024 2

Thanks @mformigo, I'll make the example more clear.

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024 1

I disagree with @hudochenkov on this one. I think that the benefits of being able to write @include media("lap") {} instead of @include media($lap...) {} are negligible, especially when considering the amount of code changes necessary to the current implementation to make it work.

Also, the fact that you have to type $lap... every time makes it clear that you're using something different than the usual syntax, so hopefully it will make it clear that you're using an alias.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

You can always use variables:

$palm: "<=phone";
$lap: ">phone", "<=tablet";

@include media( $palm ) { }
@include media( $lap ) { }

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

I'm afraid that doesn't work @joseluis (Sassmeister gist).

@hudochenkov Other libraries, like Breakpoint-sass, use that approach. I guess it wouldn't be hard to add that functionality to include-media.

I'll have a think about a possible syntax. Feel free to share your thoughts on it as well.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

Oops, you're right. The mixin doesn't accept a list as an input.

@eduardoboucas I would like to propose that the mixin detected the type of each argument. In case of a string it would use it as usual. In case of a list then it would iterate through all of the elements treating them as arguments for the mixin.

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

Sounds good to me!

from include-media.

hudochenkov avatar hudochenkov commented on May 18, 2024

Thank, @joseluis, for idea and proposing pull request :)

I think using variables as parameters is against include-media's philosophy. My idea: create new variable $aliases similar to $media-expressions (or use it) and in parse-expression function parse it, get normal parameters for media mixin and run again parse-expression with this parameters.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

Hi @hudochenkov, I'm curious about why do you think it goes against the philosophy of the project. I belive it supports the motto of using media-queries in a simple, elegant and maintainable way.

Since the ability to use variables as arguments is essentially unavoidable, as it is inherited from the Sass language itself, the idea is to increase the support for that language expectative in a transparent way. It would allow more creative ways to call the media mixin, too.

I also like how the code reads:

$breakpoints: (phone: 320px, tablet: 768px, desktop: 1024px);

// e.g. aliases using variables
$lap: ">phone", "<=tablet";

@include media( $lap ) { }


// e.g. aliases using map
$bp: (
  lap: (">phone", "<=tablet"),
  phonetop: (">phone", "<desktop")
);

@include media( $bp(lap) );

But anyhow, both ideas are not incompatible. I strongly believe the mixin would benefit immensely from being able to receive lists of parameters. But it could also be possible to provide an additional global map for "aliases" if the idea has enough support.

@eduardoboucas what do you think?

from include-media.

hudochenkov avatar hudochenkov commented on May 18, 2024

@joseluis, I've tried your addition and it's not working with case I've created this issue for. And I doesn't explain this case properly :)

For example, I have responsive website and in its SCSS I have a lot styles for tablet and mobile.

.selector {
    color: black;

    @include media(">phone", "<=tablet") {
        color: green;
    }

    @include media("<=phone") {
        color: blue;
    }
}

I don't want repeat each time ">phone", "<=tablet" so I can create aliases for this:

$aliases: (
    "lap": (
        ">phone", 
        "<=tablet"
    ),
    "palm": "<=phone"
) !default;

and use these aliases:

.selector {
    color: black;

    @include media("lap") {
        color: green;
    }

    @include media("palm") {
        color: blue;
    }
}

Suddenly I need add to some rules extra parameters:

.selector2 {
    color: #bada55;

    @include media("lap", "retina2x") {
        color: #b000b5;
    }
}

Currently your addition for variables support fails:

.selector2 {
    color: #bada55;

    @include media($lap, "retina2x") {
        color: #b000b5;
    }
}

Even when you resolve this case too, parameters syntax for media mixin wasn't be consistant: your $lap, "retina2x" vs hypothetical image-media's style "lap", "retina2x".

I'm totally support adding variables for parameters, but also want “native” include-media's support for aliases.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

@hudochenkov I understand. And now I'm with you. I see how handy it is to have a global map for aliases in the way you propose (although I like better the name $media-aliases). I'd like to hear @eduardoboucas thoughts about this.

I imagine that an alias key should take precedence over a $breakpoint key. I'll try to deliver a new PR that supports global aliases in a sensible way.

BTW Your test case works for me using the mixing from the previous PR (sm gist).

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

Sorry guys, I'm not ignoring you. I'll have some proper time tonight to dedicate to this and I'll let you know my thoughts. Thanks!

from include-media.

hudochenkov avatar hudochenkov commented on May 18, 2024

@joseluis about @include media($lap, "retina2x") { }. My bad, i've wrote retina instead retina2x in my test :) BTW, it's will be great if such errors will be handled by include-media and ignored.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

Thanks @eduardoboucas, looking forward to it.

I've just finished a working example adding support for aliases to the media mixin: (SassMeister gist).

My comments: I believe it's more logical to modify the media mixin instead of modifying the parse-expression function. This is the simplest way I see to do it. I could have created a helper function to deal with the lists but I don't think it's worth the effort yet.

@hudochenkov Yes. I also believe include-media errors could be handled better. Let's have that in mind for other PR.

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

I like the $media-aliases approach, although I'm a bit concerned that the implementation eventually becomes too complex. I think that at some point this library won't be able to please everyone. But if this in particular is useful to people without affecting the syntax that everyone else is already using, then let's do it!

from include-media.

joseluis avatar joseluis commented on May 18, 2024

> I'm a bit concerned that the implementation eventually becomes too complex.

Yes. There is always the risk of increased complexity when adding new functionality. But in this case I don't feel we are near any red line yet.

And although simplicity in the code is very valuable, I also think it is far more important to be in the side of the user times than in the side of the implementation 9 out of 10 times.

I was reading before this post about Sass design philosophy, and specially enjoyed the questionnaire they use to decide whether to include new features or not. And I believe this particular feature passes the gauntlet.

And I also know this code could be more powerful. It could be more modular, with helper functions, allowing nested lists and aliases referencing other aliases, but I'm not sold in the need for that. One nested level is simple enough, easy to understand and it surely fulfills 99% of the needs.

> If this in particular is useful without affecting the syntax (...) then let's do it!

Yes. This doesn't affect the previous syntax, and it sure opens useful ways of using it. I'll make a new PR!

from include-media.

illarionvk avatar illarionvk commented on May 18, 2024

I'm concerned too that include-media will become too complex. I like the original idea of the library, it's simple and elegant.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

good news everyone!

I just discovered how to give a lists of parameters to a Sass function/mixin and get them interpreted as lists of arguments, without modifying any of the existing code. Just by appending three dots (...) to the end of the passed argument:

$lap: ">phone", "<=tablet";

@include media($lap...) {
  color: red;
}

Example: SassMeister gist
Source: Sass 3.2, Passing a list as arguments

So there doesn't seem to be a need for the PR anymore just to support this functionality. Unless you decide you want the nicer syntax of course.

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

Brilliant! I like that.

This way we can support aliases with no changes whatsoever to the implementation.

@hudochenkov are you happy with this?

from include-media.

hudochenkov avatar hudochenkov commented on May 18, 2024

This is interesting discovery, @joseluis!

But it's look hacky. In typical responsive website stylesheet you need to type media($lap...) a lot. It's will be better to have clean and consistent syntax (media("lap")) across all stylesheet.

from include-media.

joseluis avatar joseluis commented on May 18, 2024

As much as I'd like to contribute to this project, I'm with @eduardoboucas on this one. Given this new revealing information I don't see the need anymore to add an additional layer of complexity. Even more when the nicer syntax would give less information in a first glance than the vanilla Sass syntax.

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

You did contribute, @joseluis. Thanks for the effort you put in!

I'll add an example of this use case to include-media.com.

from include-media.

hudochenkov avatar hudochenkov commented on May 18, 2024

I will not insist on adding “native” aliases to include-media.

Thank you, @eduardoboucas, for include-media. It's really flexible approach for media queries :)

Thank you, @joseluis, for all your help! I'll use your PR for aliases for my projects :)

from include-media.

mformigo avatar mformigo commented on May 18, 2024

Hey there to all,

Firstly I wish to say thanks for this awesome Sass library 😃

I got off to a late start on Sass, but I'm loving what I'm learning and it's all great 😁
And this library just makes it all so much easier to handle media queries!!

Still, this issue with all it's comments really ended up helping because despite the features description on include-media.com, the example regarding Expression aliases was, I'm sorry to say, a bit misleading.

Don't get me wrong, the example in itself is right, but the ... on the @include media($param...) {} of the example kind of gets pass the reader a bit unnoticed 😕 - at least that's my take on it. Only after coming by this issue and reading @joseluis's comment on the three dots, did I became ware of them.
I guess it's kind of like what @hudochenkov suggested and I instinctively went right for it.
But don't get me wrong, I don't mind using the ..., I agree it's perfectly acceptable instead of having to rewrite a ton of code just to be able to omit the dots - it just wouldn't be practical.

My only suggestion to @eduardoboucas is perhaps including a notice for the thee dots on the Expression aliases example on include-media.com. Maybe making it something like:

You can create aliases for expressions that you find yourself reusing a lot, no matter how complex they are. But notice the three dots following the $my-weird-bp parameter, as they are required.

Anyway, just wanted to share this bit of though that might help others when reading its documentation. Again, thank you so much for this library and continue the great work. :simple_smile:

from include-media.

eduardoboucas avatar eduardoboucas commented on May 18, 2024

Done! Thanks for flagging this issue. I hope this makes it clearer:

screen shot 2016-11-30 at 20 41 20

from include-media.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.