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

Selector Functions #1261

Closed
wants to merge 2 commits into from

Conversation

onedayitwillmake
Copy link

Please note: The code in this pull request was written by myself, and is does not represent my employer.

Redoing #1064.

This is a fresh checkout of master, and then manually went in and re-implemented the selector-functions.
This is a much simpler PR with less files modified, and only a single commit - hopefully that helps make everything more clear when merging

Adds the selector functions:

  • selector-nest($selectors...)
  • selector-append($selectors...)
  • selector-extend($selector, $extendee, $extender)
  • selector-replace($selector, $original, $replacement)
  • selector-parse($selector)
  • simple-selectors($selector)
  • selector-unify($selector1, $selector2)

Regarding the sass-spec test, there are 2 issues when calling selector-unify and selector-nest and passing in a parent selector reference.

I believe these are related to a couple of other issues cropping up around & - so I think it's safe to merge this in and tackle those as a new issue.

Known Issues from sass-spec libsass issue_963 tests:

selector-unify

selector-unify(&, '.baz, .bang'); // never makes it passed the parser
Error: invalid selector after &
        on line 14 of test_selector_unify.scss
>>   content: selector-unify(&, '.baz, .bang');
   --------------------------^

selector-nest

selector-nest('.foo', '&.bar','baz &"); // '&' before .bar and after .baz are thrown out

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.43%) to 79.21% when pulling 0514d01 on onedayitwillmake:sel_functions into 25a1d79 on sass:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-1.43%) to 79.21% when pulling 0514d01 on onedayitwillmake:sel_functions into 25a1d79 on sass:master.

mgreter added a commit to mgreter/libsass that referenced this pull request Jun 5, 2015
@@ -117,6 +118,38 @@ namespace Sass {
}
return val;
}

#define ARGSEL(argname, seltype, contextualize) get_arg_sel<seltype>(argname, env, sig, pstate, backtrace, ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

contextualize doesn't seem to be used?

@mgreter
Copy link
Contributor

mgreter commented Jun 5, 2015

Hey @onedayitwillmake, already took a quick look yesterday and looks very nice and clean. Unfortunately appveyor doesn't yet pass, which is pretty much the only thing that I see as a blocker before merging this PR as is. @xzyfer I think at this point it will be best to merge this PR to master and I will then realign my WIP branch for the 3.3 release. I guess we need a few beta rounds anyway with 3.3.

Should we create a separate release coordination issue as I did for 3.2?

@mgreter mgreter added this to the 3.3 milestone Jun 5, 2015
@onedayitwillmake
Copy link
Author

@mgreter Thank you! Yes it seems to dislike the std::function I used as a custom comparator for creating a std::set< Complex_Selector*> - I assumed I could use C++11 feature.

Looking again, i see you already have a Complex_Selector_Pointer_Compare functor I can use instead!
I will switch to that and commit :)

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.41%) to 79.23% when pulling 60cc9fc on onedayitwillmake:sel_functions into 25a1d79 on sass:master.

@mgreter
Copy link
Contributor

mgreter commented Jun 13, 2015

I have incorporated most of your changes in my latest WIP branch: #1249 - maybe check if I have missed anything! It seems to pass most of the tests and I tried to replace some functionality with more generic functions, which I added during the refactoring!
IMHO we now mostly need to tackle is_superselector in regard to :not (and other) pseudo selectors!
And once again, thanks for your contributions!

@mgreter
Copy link
Contributor

mgreter commented Jul 13, 2015

I guess this can be closed since #1249 has been merged 🎉

@mgreter mgreter closed this Jul 13, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants