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

Passing arguments from a mixin to a content block #871

Closed
5 tasks done
chriseppstein opened this issue Jul 30, 2013 · 123 comments
Closed
5 tasks done

Passing arguments from a mixin to a content block #871

chriseppstein opened this issue Jul 30, 2013 · 123 comments
Assignees
Labels
enhancement New feature or request specs written Specs have been written for the feature and at least one implementation passes them

Comments

@chriseppstein
Copy link

chriseppstein commented Jul 30, 2013


Mixin content is great and has done wonders for code implementing wrapper and context abstractions. However, there is often some setup done in the mixin and that setup needs to be available to the content when it is included. The current strategy is to use global variables, which works, but is obviously not ideal.

I'd like to introduce a way for mixins to include their content block and to pass arguments to the content.

Calling the content with arguments

In order to pass arguments to content blocks an argument list can be passed to the content directive. This accepts positional and keyword arguments for passing to a content block. Variable argument semantics are allowed.

@mixin accepts-content {
  @for $i from 1 to 5 {
    @content ($i, 360deg * $i / 5);
  }
}

Receiving arguments within a content block

A content block receives arguments using a new directive called @receive. The directive allows an argument list declaration that is the same as for functions and mixins. Positional arguments, default values, and variable arguments are all allowed.

@include accepts-content {
  @receive ($number, $hue);
  .color-#{$number} {
    background-color: hsl($hue, 75%, 90%);
  }
}
@Snugug
Copy link

Snugug commented Jul 30, 2013

I like this idea. 👍

@cimmanon
Copy link

Not crazy about the syntax for @receive, but very interested in the concept.

@chriseppstein
Copy link
Author

Comments of general distaste are not very useful without explanations and, ideally, alternative suggestions.

@gabrielnau
Copy link

It's really fine !

  1. global variables are not ideal like you said
  2. it explicits what variables the nested block needs

@HamptonMakes
Copy link
Member

At first blush, I like it.

How about @Bind instead of @receive... maybe that's too nerdy?

On Thu, Aug 1, 2013 at 2:32 PM, Gabriel Nau notifications@github.comwrote:

It's really fine !

  1. global variables are not ideal like you said
  2. it explicits what variables the nested block needs


Reply to this email directly or view it on GitHubhttps://github.com//issues/871#issuecomment-21971416
.

@ghost
Copy link

ghost commented Aug 1, 2013

Could always pull a JavaScript and make an $arguments variable. Or an arguments($n) function.

@chriseppstein
Copy link
Author

The problem with $arguments is that it only work for positional arguments. I'd like consistency with all other argument passing and definitions. With arguments(...) I really don't like the idea of a function that sets instead of receives arguments.

Re: Naming, I'm not a fan of @bind for this. Maybe @content-arguments is better; it show's it's related to the @content directive.

@nex3
Copy link
Contributor

nex3 commented Aug 3, 2013

I'm not a fan of @receive either; the directive format is not a good fit for receiving arguments. That said, I do want this feature, and I can't think of any other syntax that would be better.

@HamptonMakes
Copy link
Member

I guess I'm not a fan of receive due to it being commonly misspelled. Just
brainstorming....

@take
@pass
@get
@->
@vars
@name

On Friday, August 2, 2013, Nathan Weizenbaum wrote:

I'm not a fan of @receive either; the directive format is not a good fit
for receiving arguments. That said, I do want this feature, and I can't
think of any other syntax that would be better.


Reply to this email directly or view it on GitHubhttps://github.com//issues/871#issuecomment-22046560
.

@chriseppstein
Copy link
Author

I think I like @content-arguments best so far.

@HamptonMakes
Copy link
Member

Hrrm... as I look at the use case more, this seems like something we think
would be pretty regularly used. Aka, I could make a Sass library that uses
this as a common API for interaction. And, with that in mind, this overall
approach of making it a directive line is odd to me. I'd much rather it
somehow work into the syntax more directly.

We could steal some Ruby syntax!

@include accepts-content { |$number, $hue|
  .color-#{$number} {

    background-color: hsl($hue, 75%, 90%);

  }

}

On Sun, Aug 4, 2013 at 11:30 AM, Chris Eppstein notifications@github.comwrote:

I think I like @content-arguments best so far.


Reply to this email directly or view it on GitHubhttps://github.com//issues/871#issuecomment-22076323
.

@cimmanon
Copy link

cimmanon commented Aug 5, 2013

The part I'm not a fan of is how much additional code this adds from a reuse standpoint. Using global variables is worse when it comes to authoring the mixins, but more convenient for using them. Shorter is better. If it must be a keyword, @receive or @args is better than @receive-content or @content-arguments, even though they may be better descriptors.

Borrowing a bit from Haskell here:

@mixin foo($a, $b) {
    @content (darken($a), lighten($b), blend($a, $b));
}

.foo {
    @include foo(red, blue) -> ($c, $d, $e) {
        @include background: $e;
        @include background-image(linear-gradient($c, $d));
    }
}

Using a Ruby style syntax as @hcatlin suggests would also be acceptable.

@Anahkiasen
Copy link

I don't get where the red and blue come from in the above example ?

@cimmanon
Copy link

cimmanon commented Aug 5, 2013

The foo() mixin takes 2 colors as arguments.

@nex3
Copy link
Contributor

nex3 commented Aug 9, 2013

I'm more of a fan of @cimmanon's example than any of the others I've seen. Placing the arguments before the { puts them firmly in the context of @include, both from the parser's standpoint and the reader's standpoint.

I'm not a big fan of ->. Sass tends to use words over symbols where possible, so something like @include foo(red, blue) with ($c, $d, $e) or taking ($c, $d, $e) might be better.

@chriseppstein
Copy link
Author

I actually really like the with or taking syntax and I'm sad I didn't think of it first ;)

Leaning towards taking. I also like receiving, but as @hcatlin points out people always misspell that. However, we could also support recieving and print out a message mocking their inability to spell.

@nex3
Copy link
Contributor

nex3 commented Aug 9, 2013

I don't like how much longer receiving is, either.

@cimmanon
Copy link

What about as (from SQL)? returning (also SQL) or returns might make sense.

@ghost
Copy link

ghost commented Aug 11, 2013

How about @include foo(red, blue) using ($a, $b, $c) ?

@nex3
Copy link
Contributor

nex3 commented Aug 11, 2013

I think as is too unclear, and returns/returning are confusing given that @return already exists and does something different.

I like using. That may be my favorite so far.

@chriseppstein
Copy link
Author

using 👍

Objections to this for 3.3? Would really clean up some compass code.

@nex3
Copy link
Contributor

nex3 commented Aug 14, 2013

I don't want any new features in 3.3. I want to get it out the door as soon as the existing features for it are done.

@chriseppstein
Copy link
Author

ok.

@ghost ghost mentioned this issue Aug 23, 2013
@ghost
Copy link

ghost commented Aug 23, 2013

From my short-lived #894, since the @mixin is defining variables to be bound in the content block scope, the content block scope should automatically bind them, but do so with an implicit !default and a warning.

Optionally a using keyword could allow the @mixin caller to rename variables before they are bound ( and thus wouldn't issue a warning )

$bar: already-defined;

// would normally bind 
// $bar to some value
@include foo ($arg) using ($bar as $qux) {
    background-color: $qux;
}

I just don't see a reason to force all variables to be explicitly named ( as previously discussed ); it's not particularly magical if done implicitly as the mixin consumers will need to refer to argument documentation regardless.

</2-cents>

@nex3
Copy link
Contributor

nex3 commented Aug 24, 2013

I think it's important that all the variables are named at the call site. It makes it possible to determine where each variable is coming from by inspecting the code around it.

@elisechant
Copy link

@chriseppstein passing arguments to @content would be incredibly useful. Can we prioritize this issue?

@cjcheshire
Copy link

Yes please! I came across this issue recently and would love this to help
tidy up some loops I've had to duplicate.

On Tuesday, September 17, 2013, Elise Chant wrote:

@chriseppstein https://github.com/chriseppstein passing arguments to
@content would be incredibly useful. Can we prioritize this issue?


Reply to this email directly or view it on GitHubhttps://github.com//issues/871#issuecomment-24565751
.

@elisechant
Copy link

For anyone interested, my workaround at the moment requires a repeating loop:

_config.scss

// Category theme settings
// ------------------------------------
// store config in an associated array so we can loop through
// and correctly assign values
//
// Use this like this:
// Note - The repeated loop cannot be abstracted to a mixin becuase
// sass wont yet allow us to pass arguments to the @content directive
// Place the loop inside the selector

$themes:
    'category-a' $color-quaternary,
    'catgeory-b' $color-tertiary,
    'catgeory-c' $color-secondary
;

_breadcrumb.scss

#el {
    @each $theme in $themes {
        $class: nth($theme, 1);
        $color-prop: nth($theme, 2);

        .#{$class} & {
            border: 1px solid $color-prop;
        }
    }
}

@cjcheshire
Copy link

Yea I have something similar but like you say you can't do the loop within
the mixin - yet :)

I wanted the loop within a mixin as then it's tidier and cleans the code up
so much.

I'm happy it's being discussed - I thought about it the other day and
figured I was asking for something mad.

On Wednesday, September 18, 2013, Elise Chant wrote:

For anyone interested, my workaround at the moment requires a repeating
loop:
_config.scss

// Category theme settings
// ------------------------------------
// store config in an associated array so we can loop through
// and correctly assign values
//
// Use this like this:
// Note - The repeated loop cannot be abstracted to a mixin becuase
// sass wont yet allow us to pass arguments to the @content directive
// Place the loop inside the selector

$themes:
'category-a' $color-quaternary,
'catgeory-b' $color-tertiary,
'catgeory-c' $color-secondary
;

_breadcrumb.scss

#el {
@each $theme in $themes {
$class: nth($theme, 1);
$color-prop: nth($theme, 2);

    .#{$class} & {
        border: 1px solid $color-prop;
    }
}

}


Reply to this email directly or view it on GitHubhttps://github.com//issues/871#issuecomment-24631853
.

xzyfer added a commit to sass/libsass that referenced this issue Nov 25, 2018
xzyfer added a commit to sass/libsass that referenced this issue Nov 25, 2018
xzyfer added a commit to sass/libsass that referenced this issue Nov 25, 2018
xzyfer added a commit to sass/libsass that referenced this issue Nov 25, 2018
xzyfer added a commit to sass/libsass that referenced this issue Nov 25, 2018
xzyfer added a commit to sass/libsass that referenced this issue Nov 26, 2018
@xzyfer
Copy link

xzyfer commented Nov 26, 2018

This has also landed in LibSass. Should this issue be closed?

@abdullahseba
Copy link

How long would this typically take to trickle into node-sass?
Its just what I need!

@nex3 nex3 closed this as completed Nov 26, 2018
@stereokai
Copy link

Woohoo! Been on the lookout for this for 3 years! Big thanks!

@CaelanStewart
Copy link

Goes to show, good things come to those who wait.

Nice work guys!

@nex3
Copy link
Contributor

nex3 commented Dec 4, 2018

@xzyfer When you close out these issues, can you let me know the first LibSass release number these features are expected to land in? That way I can update the new reference docs' implementation tracker.

@entozoon
Copy link

entozoon commented Dec 5, 2018

And the subsequent node-sass release if anyone catches it. Then we can put this whole 5+ year ordeal to bed 👯 🍾

@leefernandes
Copy link

has this not landed on node-sass?

@nex3
Copy link
Contributor

nex3 commented Jul 24, 2019

@itsleeowen it landed in sass/libsass#2761

@KaelWD
Copy link

KaelWD commented Jul 24, 2019

Yeah and node-sass still isn't using it.

@sidelong44
Copy link

When to wait for an update for node-sass?
This is a very important feature.
I'm really looking forward to it.

@leefernandes
Copy link

leefernandes commented Jul 24, 2019

the @content argument examples work w/ node-sass on my end with these updates: sass/node-sass#2714

node-sass mocha tests don't pass, I'm not sure how to reconcile node-sass tests against libsass changes between Nov 11, 2018 and libsass#2b8a17a. Unless I'm misunderstanding the failing node-sass tests, I'm not confident that node-sass having it's own set of style tests is a good idea, it should probably depend on libsass tests if it's not.

5935 passing (18s)
753 pending
143 failing

also... @content arguments are AMAZING, thanks you!

😺

@nex3
Copy link
Contributor

nex3 commented Jul 24, 2019

I don't know what Node Sass's release schedule is—you'd have to follow along on sass/node-sass#2685.

@johnnyBira
Copy link

@itsleeowen How? I'm using the latest version of node-sass and it seams to be using libSass 3.5.4, ie. no @content arguments from what I understand it.

@esr360
Copy link

esr360 commented Jan 1, 2020

I'm using latest version of Node-Sass (4.13.0) and this doesn't seem to be working, but it's still great news that the feature is now in Sass. I'm struggling to find where to follow for updates for Node-Sass though. This thread mentions to follow a PR for updates, but the PR was already merged.

@nex3
Copy link
Contributor

nex3 commented Jan 1, 2020

Node Sass versions are released later (sometimes substantially later) than the corresponding LibSass versions. I'm not sure if there's a central place to look for them. Maybe @xzyfer or @nschonni would know?

Alternately, you could use Dart Sass (https://www.npmjs.com/package/sass) which definitely has this feature.

@katerlouis
Copy link

What's the status on this one?

@nex3
Copy link
Contributor

nex3 commented Apr 28, 2020

As the comment above indicates, this is implemented everywhere but may not yet be released in Node Sass.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request specs written Specs have been written for the feature and at least one implementation passes them
Projects
None yet
Development

No branches or pull requests