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

Refactor @content arguments specs #1543

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Refactor @content arguments specs #1543

wants to merge 1 commit into from

Conversation

jathak
Copy link
Member

@jathak jathak commented Jun 5, 2020

No description provided.

@jathak jathak requested a review from nex3 June 5, 2020 20:03
@jathak jathak changed the title Refactor `@content arguments specs Refactor @content arguments specs Jun 5, 2020
<===>
================================================================================
<===> syntax/arglist/invalid/input.scss
@mixin mixin {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@mixin mixin {
@mixin a {

Super nitty, but these specs were written before the style guide was locked in, so they don't follow DO use single-letter names for irrelevant placeholders. Since you're touching them anyway, it's probably worth updating them to fully follow the style guide.

Comment on lines +1 to +15
<===> no_parens/no_using/input.scss
@mixin mixin {
@content;
}

a {
@include mixin {
x: y;
}
}

<===> no_parens/no_using/output.css
a {
x: y;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems strange to have a spec under content/arguments that calls a mixin without passing any arguments to its content block.

Comment on lines +1 to +11
<===> with_defaults/nothing_passed/input.scss
@mixin mixin {
@content;
}

a {
@include mixin using ($arg1: value1, $arg2: value2) {
arg1: $arg1;
arg2: $arg2;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems redundant with the default_using specs in none.hrx.

Comment on lines +126 to +148
<===> with_splat/both_passed/input.scss
@mixin mixin {
@content(value1, $arg2: value2);
}

a {
@include mixin using ($args...) {
positional: inspect($args);
keywords: inspect(keywords($args));
}
}

<===> with_splat/both_passed/output.css
a {
positional: (value1,);
keywords: (arg2: value2);
}

<===> with_splat/both_passed/output-libsass.css
a {
positional: value1;
keywords: (arg2: value2);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably also a good idea to add a spec covering passing an ArgumentList object that contains both keywords and positional args to a @content.

Comment on lines +4 to +26
<===>
================================================================================
<===> no_argument/input.scss
$var: top-level;

@mixin mixin($var) {
mixin-var-before: $var;
@content;
mixin-var-after: $var;
}

a {
@include mixin(mixin-argument) {
content-var: $var;
}
}

<===> no_argument/output.css
a {
mixin-var-before: mixin-argument;
content-var: top-level;
mixin-var-after: mixin-argument;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, since this isn't actually declaring or passing content arguments, it seems like it doesn't belong here.

@connorskees
Copy link
Contributor

Hello,

I've come across an interesting case when compiling Bootstrap that may be relevant here.

The minimized version is

@mixin foo($arg) {
  @include bar {
    color: $arg;
  }
}

@mixin bar {
  @include baz {
    @content;
  }
}

@mixin baz {
  @content;
}

@mixin font-size($value) {
  @include foo($value);
}

a {
  @include font-size(1rem);
}

which I suppose can be thought of as a triple chain of @content.

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 this pull request may close these issues.

3 participants