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

Support parent selector & in values / and/or interpolations #548

Closed
gionkunz opened this issue Oct 17, 2014 · 38 comments · Fixed by #966
Closed

Support parent selector & in values / and/or interpolations #548

gionkunz opened this issue Oct 17, 2014 · 38 comments · Fixed by #966

Comments

@gionkunz
Copy link

Hi

First I'd like to thank you for your awesome work!

I've put together a small BEM Sass library http://codepen.io/gionkunz/pen/rkswl and there are problems in libsass with the parent selector. It looks like there is an undocumented feature in RubySass that the parent selector & can also be converted to a selector string. I believe internally its a list of lists if I'm not mistaken. So in .test { .abc, .def { &:before { content: "." } } } & would be stored as list of lists ( ( ".test .abc" ) , (".test .def") ). In RubySass when the parent selector & appears where a value is expected it gets converted to a string http://sass-lang.com/documentation/Sass/Script/Functions.html#selector_functions .

.parent-sel-value {
  font-family: &;
  .parent-sel-interpolation {
    font-family: #{&};
     .parent-sel-value-concat {
        font-family: "Current parent: " + &;
     }
  }
}

Gets processed to:

.parent-sel-value {
  font-family: .parent-sel-value;
}
.parent-sel-value .parent-sel-interpolation {
  font-family: .parent-sel-value .parent-sel-interpolation;
}
.parent-sel-value .parent-sel-interpolation .parent-sel-value-concat {
  font-family: "Current parent: .parent-sel-value .parent-sel-interpolation .parent-sel-value-concat";
}

This feature is particularly interesting for libraries and frameworks that need to have more control over selectors during nesting. I'd really love to have this in libsass too.

I did not find any spec in https://github.com/sass/sass-spec concerning this undocumented feature and I'd also help where I can to get this in.

Cheers
Gion

@HamptonMakes
Copy link
Member

Ohhh... this is a good one. Thanks for the useful bug report. I've marked this as needs test and we'll get it in sass-spec soon.

@xzyfer
Copy link
Contributor

xzyfer commented Oct 29, 2014

Spec added sass/sass-spec#103

@bitmensch
Copy link

Just stumbled upon this issue. It seems this is also a blocker for using http://www.csstyle.io/ with libsass.

@ebrentnelson
Copy link

I second benjixx's comment. libsass is choking on csstyle. I've been trying to figure out why.

We have something like this:

 23 // allow parts to respond to options on parents
 24 @mixin whenComponentOption($option){
 25   $index: str-index(#{&}, "__") - 1;
 26   $component: str-slice(#{&}, 0, $index);
 27
 28   #{$component}.\--#{$option} & {
 29     @content;
 30   }
 31 }

That throw an error like this:

node_modules/csstyle/csstyle:25: error reading values after

I have tracked it down to the ampersand on line 25, but have been unsuccessful at fixing it in csstyle alone. (Note: In my case I am using sass-loader for webpack that uses libsass under the hood---I am pretty sure.)

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Hi guys,

I'd like to try to implement this in my own fork but not quite sure how to get the parent selector and then translate that to an expression.

It seems that the if block in parse_simple_selector_sequence in the Parser isn't what I thought. I thought that that would give me a reference to the parent selector (based on the comments). If it does, I"m not sure how to reference that in the parse_value method when it tries to parse an assignment statement.

Any help would be appreciated.

Also it seems that Selectors can't be Expressions, which is fine because I can use Selector_Schema to get the string value, right? I think what I'm really looking for is a way to get the environment/context when the parser is parsing a given statement.

Thanks in advance.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

If you're willing to wait I plan to tackle this over the weekend.

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Thats great news! Thank you. Ive spent some trying to figure it out and
have been stymied. I think i will wait till you implement it but the
curiosity is killing me on how to implement it. I read through the wiki on
how to develop for libsass and i still was not sure how to understand some
of the domain language. Would you mind explaining to me how to implement
this? The curiosity is killing me.
2015. 1. 20. 오후 8:19에 "Michael Mifsud" notifications@github.com님이 작성:

If you're willing to wait I plan to tackle this over the weekend.


Reply to this email directly or view it on GitHub
#548 (comment).

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

The general idea is you want want parse_value to lex just the & symbol (we don't care about selectors) and return a reference selector AST node. Then the visitors will take care of the selector substitution for you.

If you want to give it a shot I'll take a look at the PR.

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Thank you~! So that was my first approach but parse_value returns an expression and I didn't know how to create an Expression from a Selector_Reference. The AST value isn't accessible from a Selector_Reference object....

Also i'd love to continue to contribute to this so I'll take a shot and send you a PR when I think I have it working....

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

So I am trying to return this:
Selector_Reference* sel = new (ctx.mem) Selector_Reference(pstate);
return new (ctx.mem) Expression(sel->pstate());

And when i output the & reference, I get an empty string. So I think I must not be returning the correct value....

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

Yeah I had a look, it's a little more complicated than expected. You need to expand the Selector_Reference with contextualize.

This gets tricky because parse_value wants to return an Expression, and Expressions are evaluated in the eval visitor which doesn't know about the selector stack.

Some wire will need to be crossed to get crossed. My gut feeling is that the selector expanding logic will need to be factored out of the Ruleset expand visitor into it's own operator.

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Ah, okay so that's what I thought. I was hoping for a simpler answer, which was simply returning the right value in parse_value, but like you said, there was no way to access the member I needed, or rather, the member simply doesn't exist.

When you say expand the Selector_Reference with contextualize, what do you mean? Do you mean have Selector_Reference have a reference to a contextualize member or to inherit from it?

I can work on refactoring the selector expanding logic into its own operator.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

If you take a look at the Ruleset operator in the expand visitor you'll
see how selectors are resolved (via the contextualize class). That
resolution process takes the current selector stack and figures out the
value of &.
On 21 Jan 2015 17:47, "Eric Kimn" notifications@github.com wrote:

Ah, okay so that's what I thought. I was hoping for a simpler answer,
which was simply returning the right value in parse_value, but like you
said, there was no way to access the member I needed, or rather, the member
simply doesn't exist.

When you say expand the Selector_Reference with contextualize, what do
you mean? Do you mean have Selector_Reference have a reference to a
contextualize member or to inherit from it?

I can work on refactoring the selector expanding logic into its own
operator.


Reply to this email directly or view it on GitHub
#548 (comment).

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Wouldn't I have to

a) make Selector_Reference inherit from Expression
b) Have the expand visitor apply the Contextualize to the Selector_Reference for the Assignment visitor method?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

Unfortunately it's not assignments. The & is an expression that evaluates
to a list of selectors. As a result it can be used many places a list can
i.e. assignments, selectors, function arguments
On 21 Jan 2015 19:05, "Eric Kimn" notifications@github.com wrote:

Wouldn't I have to

a) make Selector_Reference inherit from Expression
b) Have the expand visitor apply the Contextualize to the
Selector_Reference for the Assignment visitor method?


Reply to this email directly or view it on GitHub
#548 (comment).

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

ah...the spec that was written only seemed to account for the assignment case....so this means that I'd have to apply this to ALL the visitor methods??? that kinda sucks....there should be a better way.

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

You should need to create a single operator on the the expand and eval
visitors for handling Selector and some wrapper Expression node. This
is mostly guess work, I know more when I dig into it.
On 21 Jan 2015 19:34, "Eric Kimn" notifications@github.com wrote:

ah...the spec that was written only seemed to account for the assignment
case....so this means that I'd have to apply this to ALL the visitor
methods??? that kinda sucks....there should be a better way.


Reply to this email directly or view it on GitHub
#548 (comment).

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Ah, that's what you mean by the stacks being different...the eval visitor is for expressions primarily and the expand visitor is primarily for Statements?

It seems like I need to create a SelectorExpression, like the MediaQuery_Expression class....what do you think?

@xzyfer
Copy link
Contributor

xzyfer commented Jan 21, 2015

Exactly. Only the expand visitor know how to hydrate the & expression. My
gut feeling is you'll need a new Expression subclass node in the mix.
On 21 Jan 2015 19:47, "Eric Kimn" notifications@github.com wrote:

Ah, that's what you mean by the stacks being different...the eval visitor
is for expressions primarily and the expand visitor is primarily for
Statements?


Reply to this email directly or view it on GitHub
#548 (comment).

@ekskimn
Copy link

ekskimn commented Jan 21, 2015

Hmm....so there's a dilemma where the expand visitor takes the eval visitor as a constructor parameter because it uses it to eval arguments, which is fine, but if I were to make the eval visitor take the expand visitor as a construction parameter then that would be circular....do I need a new visitor class to submit to both classes?

@xzyfer
Copy link
Contributor

xzyfer commented Feb 25, 2015

No worries @ekskimn I'm rather pre-occupied myself atm.

We appreciate all your work.

@ekskimn
Copy link

ekskimn commented Feb 26, 2015

Hey @xzyfer
Just to let you know where i am with things...i have the first test case figured out but when the parent selector is in another statement such as an interpolation, i'm having a hard doing that without the contextualizer being in the eval visitor. The problem is that the contextualizer takes the eval as a parameter.

What do you recommend?

@xzyfer
Copy link
Contributor

xzyfer commented Feb 26, 2015

I'm sorry but it's just really hard to visualise with out seeing the code
so far. It's perfectly fine to put up a PR that's not perfect.
On 26 Feb 2015 19:23, "Eric Kimn" notifications@github.com wrote:

Hey @xzyfer https://github.com/xzyfer
Just to let you know where i am with things...i have the first test case
figured out but when the parent selector is in another statement such as an
interpolation, i'm having a hard doing that without the contextualizer
being in the eval visitor. The problem is that the contextualizer takes the
eval as a parameter.

What do you recommend?


Reply to this email directly or view it on GitHub
#548 (comment).

@ekskimn
Copy link

ekskimn commented Mar 17, 2015

PR to you @xzyfer ...like the comments say, i have the declaration and the binary operations working but the interpolation isn't...though it really should and not quite sure why it wouldn't....I'm still looking at that, but if you get a chance take a look at the PR

@xzyfer
Copy link
Contributor

xzyfer commented Mar 17, 2015

Thanks for your work, we really appreciate it. FWIW there's a comprehensive spec suit for this feature now at https://github.com/sass/sass-spec/tree/master/spec/libsass-todo-tests/selectors.

ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
edits responding to comments for append_string
typo for the broken line comma
added contextualize_eval to the visual studio project file.
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 17, 2015
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 18, 2015
includes from parent selector:
158a5af Support parent selector & in values / and/or interpolations sass#548
d10b572 Merge branch 'master' into parent-selector
829fef7 merge from master to parent-selector
e1f78c0 Merge commit 'dea27e5e3cc61a0d9f00ffa3f598a1080c6502d6' into parent-selector
ef73c1b issue 548
776e997 sass#548 Fixed the interpolation~!
324b14f sass#548 Fixing orphaned variable
1c15df0 sass#548 fixes for eval calling var->perform(this) edits responding to comments for append_string typo for the broken line comma added contextualize_eval to the visual studio project file.
d9090b3 sass#548 fixed the set fault tests...
92e3f0b sass#548 removing commented out code thanks @mgreter.
98977d8 sass#548 removed commented code
5b6d06a sass#548 applied @xzyfer patch. Some clean up of commented out code
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 18, 2015
includes from parent selector:
158a5af Support parent selector & in values / and/or interpolations sass#548
d10b572 Merge branch 'master' into parent-selector
829fef7 merge from master to parent-selector
e1f78c0 Merge commit 'dea27e5e3cc61a0d9f00ffa3f598a1080c6502d6' into parent-selector
ef73c1b issue 548
776e997 sass#548 Fixed the interpolation~!
324b14f sass#548 Fixing orphaned variable
1c15df0 sass#548 fixes for eval calling var->perform(this) edits responding to comments for append_string typo for the broken line comma added contextualize_eval to the visual studio project file.
d9090b3 sass#548 fixed the set fault tests...
92e3f0b sass#548 removing commented out code thanks @mgreter.
98977d8 sass#548 removed commented code
5b6d06a sass#548 applied @xzyfer patch. Some clean up of commented out code
ekskimn pushed a commit to ekskimn/libsass that referenced this issue Mar 18, 2015
@gionkunz
Copy link
Author

Awesome!

@gionkunz
Copy link
Author

Great effort guys 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants