-
Notifications
You must be signed in to change notification settings - Fork 189
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
Comments
You can always use variables:
|
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. |
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. |
Sounds good to me! |
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 |
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? |
@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 $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 I'm totally support adding variables for parameters, but also want “native” include-media's support for aliases. |
@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 I imagine that an alias key should take precedence over a BTW Your test case works for me using the mixing from the previous PR (sm gist). |
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! |
@joseluis about |
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. |
I like the |
> 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! |
I'm concerned too that |
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 (
Example: SassMeister gist 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. |
Brilliant! I like that. This way we can support aliases with no changes whatsoever to the implementation. @hudochenkov are you happy with this? |
This is interesting discovery, @joseluis! But it's look hacky. In typical responsive website stylesheet you need to type |
I disagree with @hudochenkov on this one. I think that the benefits of being able to write Also, the fact that you have to type |
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. |
You did contribute, @joseluis. Thanks for the effort you put in! I'll add an example of this use case to include-media.com. |
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 :) |
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 😁 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 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:
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: |
Thanks @mformigo, I'll make the example more clear. |
Is there any ability for declarations aliases. For example, I have a lot of phone only styles. I prefer to specify something like:
rather than:
Or for tablet only I want something like:
instead of:
Sure I can use mixins aliases:
but it's not so flexible if I, for example, want add "retina" declaration for one particular rule.
The text was updated successfully, but these errors were encountered: