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

[3.2] Bubble media queries #800

Merged
merged 4 commits into from
Jan 5, 2015
Merged

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Jan 4, 2015

This PR implements bubbling and merging of media queries.

Fixes #185. Specs added sass/sass-spec#215.

To get this over the line I enlisted the help of @hugogiraudel to buff up our @media bubbling spec coverage.

TODO

As a side affect of these fixes the following tests an outstanding @extends spec now also passes.

Bonus

This the cssize visitor added will also for the basis of @at-root!!!


This PR is ready to go for 3.2!

@xzyfer xzyfer mentioned this pull request Jan 4, 2015
7 tasks
@xzyfer xzyfer added this to the 3.2 milestone Jan 4, 2015
@xzyfer xzyfer mentioned this pull request Jan 4, 2015
8 tasks
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 4, 2015

@mgreter any ideas why the AUTOTOOLS=yes COVERAGE=no BUILD=shared would be failing?

I'm not familiar with autotools, and it onl fails on the shared build. This is the only thing holding this PR up.

https://travis-ci.org/sass/libsass/jobs/45832632

@QuLogic
Copy link
Contributor

QuLogic commented Jan 4, 2015

You have fallback_impl defined as inline in the .cpp file, but declared without inline in the .hpp file. The inline definition is used in the .cpp, but since the .hpp doesn't have the definition, it's undefined for anything that uses it, e.g, the fallback method.

In short, drop the inline and it works , though I'm uncertain why it doesn't affect eval.?pp... If you check the symbols, then you see that the one from Eval doesn't exist:

$ nm .libs/libsass.so | c++filt | grep 'fallback_impl'
000000000008af80 T Sass::Contextualize::fallback_impl(Sass::AST_Node*)
0000000000161be0 T Sass::To_C::fallback_impl(Sass::AST_Node*)
00000000000cd6a0 T Sass::Cssize::fallback_impl(Sass::AST_Node*)
00000000000b3ae0 W Sass::Expand::fallback_impl(Sass::AST_Node*)

so I think it's unused and then optimized out.

@xzyfer xzyfer force-pushed the feat/bubble-media-queries-2 branch from bfbf056 to db481cb Compare January 4, 2015 11:06
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 4, 2015

Success!! Thanks for the find @QuLogic!

I'll clean up this PR tomorrow.

@xzyfer xzyfer force-pushed the feat/bubble-media-queries-2 branch from db481cb to 01718b1 Compare January 4, 2015 23:40
@xzyfer xzyfer force-pushed the feat/bubble-media-queries-2 branch 4 times, most recently from f649e4b to 8d0a12f Compare January 5, 2015 01:42
@xzyfer xzyfer force-pushed the feat/bubble-media-queries-2 branch from c1eff65 to c79ef9f Compare January 5, 2015 02:46
@coveralls
Copy link

Coverage Status

Coverage increased (+0.25%) when pulling c79ef9f on xzyfer:feat/bubble-media-queries-2 into d679128 on sass:master.

xzyfer added a commit that referenced this pull request Jan 5, 2015
@xzyfer xzyfer merged commit 31d2feb into sass:master Jan 5, 2015
QuLogic added a commit to QuLogic/libsass that referenced this pull request Jan 5, 2015
This should avoid inconsistent errors like the one that showed up in
PR sass#800.
@sebastian-steinmann
Copy link

Any ETA on getting this into a release?

Stuck with slow ruby-sass because of this bug.

@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 12, 2015

This will land in 3.2 likely some time in Feb.

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.

Nested @media rules render different from Ruby SASS
4 participants