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

Update documentation for declaration aliases #26

Closed
hudochenkov opened this issue Feb 15, 2015 · 24 comments
Closed

Update documentation for declaration aliases #26

hudochenkov opened this issue Feb 15, 2015 · 24 comments

Comments

@hudochenkov
Copy link

Is there any ability for declarations aliases. For example, I have a lot of phone only styles. I prefer to specify something like:

@include media("palm") { }

rather than:

@include media("<=phone") { }

Or for tablet only I want something like:

@include media("lap") { }

instead of:

@include media(">phone", "<=tablet") { }

Sure I can use mixins aliases:

@mixin lap {
    @include media(">phone", "<=tablet") {
        @content;
    }
}

div {
    @include lap {
        color: red;
    }
}

but it's not so flexible if I, for example, want add "retina" declaration for one particular rule.

@joseluis
Copy link

You can always use variables:

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

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

@eduardoboucas
Copy link
Owner

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.

@joseluis
Copy link

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.

@eduardoboucas
Copy link
Owner

Sounds good to me!

@hudochenkov
Copy link
Author

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.

@joseluis
Copy link

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?

@hudochenkov
Copy link
Author

@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.

@joseluis
Copy link

@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).

@eduardoboucas
Copy link
Owner

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!

@hudochenkov
Copy link
Author

@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.

@joseluis
Copy link

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.

@eduardoboucas
Copy link
Owner

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!

@joseluis
Copy link

> 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!

@illarionvk
Copy link

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

@joseluis
Copy link

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.

@eduardoboucas
Copy link
Owner

Brilliant! I like that.

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

@hudochenkov are you happy with this?

@hudochenkov
Copy link
Author

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.

@eduardoboucas
Copy link
Owner

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.

@joseluis
Copy link

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.

@eduardoboucas
Copy link
Owner

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

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

@hudochenkov
Copy link
Author

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 :)

@mformigo
Copy link

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:

@eduardoboucas eduardoboucas reopened this Nov 29, 2016
@eduardoboucas eduardoboucas changed the title Aliases for declarations Update documentation for declaration aliases Nov 29, 2016
@eduardoboucas
Copy link
Owner

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

@eduardoboucas
Copy link
Owner

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

screen shot 2016-11-30 at 20 41 20

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 a pull request may close this issue.

5 participants