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 #790

Closed
wants to merge 7 commits into from

Conversation

xzyfer
Copy link
Contributor

@xzyfer xzyfer commented Dec 30, 2014

This PR implements bubbling and merging of media queries (#185).

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

TODO

This PR also smartens the logic around when to output some types blocks i.e. never output a ruleset that only has unresolved %placeholder selectors. I also fixes some inconsistencies in the nested output for media queries.

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 force-pushed the feat/bubble-media-queries branch from 8f40e3a to 50ac1c0 Compare December 30, 2014 11:06
@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch 2 times, most recently from 75dbc16 to 778af5b Compare December 31, 2014 07:29
@xzyfer xzyfer changed the title [WIP] Bubble media queries [DO NOT MERGE] Bubble media queries Dec 31, 2014
@xzyfer xzyfer added this to the 3.2 milestone Dec 31, 2014
@xzyfer
Copy link
Contributor Author

xzyfer commented Dec 31, 2014

I appear to be having issues with the autotools builds https://travis-ci.org/sass/libsass/jobs/45523757

@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch from 778af5b to b2f67f3 Compare December 31, 2014 07:58
@mgreter
Copy link
Contributor

mgreter commented Dec 31, 2014

Did you add your new source files to Makefile.am?

@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch from b2f67f3 to c85248b Compare January 1, 2015 01:46
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 1, 2015

You're right I missed that file. That got me closer now I'm getting

./.libs/libsass.so: undefined reference to `Sass::Cssize::fallback_impl(Sass::AST_Node*)'

with the AUTOTOOLS=yes COVERAGE=no BUILD=shared build. I'm not sure what I'm missing. I'll keep digging.

@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch from c85248b to 7bea4a9 Compare January 1, 2015 02:51
@xzyfer xzyfer changed the title [DO NOT MERGE] Bubble media queries [3.2] Bubble media queries Jan 1, 2015
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 1, 2015

This is as good as it is gets. It's ready to go in 3.2 as soon as I can track down the compilation error in AUTOTOOLS=yes COVERAGE=no BUILD=shared

@xzyfer xzyfer mentioned this pull request Jan 1, 2015
@QuLogic
Copy link
Contributor

QuLogic commented Jan 1, 2015

Fails in Windows as well:

context.obj : error LNK2019: unresolved external symbol
"private: class Sass::Statement * __thiscall Sass::Cssize::fallback_impl(class Sass::AST_Node *)"
(?fallback_impl@Cssize@Sass@@AAEPAVStatement@2@PAVAST_Node@2@@Z) referenced in function
"public: class Sass::Statement * __thiscall Sass::Cssize::fallback<class Sass::AST_Node *>(class Sass::AST_Node *)"
(??$fallback@PAVAST_Node@Sass@@@Cssize@Sass@@QAEPAVStatement@1@PAVAST_Node@1@@Z) [C:\projects\libsass\win\libsass.vcxproj]

so maybe this points you in the right direction.

vector<Block*> block_stack;
vector<Statement*> p_stack;

Statement* fallback_impl(AST_Node* n);
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably make this public or protected, I think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure why this cause compilation to fail on only some platform though. More this file is just a modified version of eval.hpp

Which declares this as protected.

@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch 4 times, most recently from 78fa418 to 8e77a8c Compare January 4, 2015 05:24
@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch from 8e77a8c to 1751f20 Compare January 4, 2015 05:27
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.27%) when pulling 1751f20 on xzyfer:feat/bubble-media-queries into 5045bce on sass:master.

@xzyfer xzyfer force-pushed the feat/bubble-media-queries branch from 1751f20 to f69a76b Compare January 4, 2015 07:59
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) when pulling f69a76b on xzyfer:feat/bubble-media-queries into 5045bce on sass:master.

This was referenced Jan 4, 2015
@xzyfer
Copy link
Contributor Author

xzyfer commented Jan 4, 2015

Superseded by #800.

@xzyfer xzyfer closed this Jan 4, 2015
@xzyfer xzyfer removed this from the 3.2 milestone Jan 4, 2015
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.

4 participants