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

Ensure correct output order of compound selectors #3086

Merged
merged 2 commits into from
May 1, 2020

Conversation

mgreter
Copy link
Contributor

@mgreter mgreter commented May 1, 2020

Fixes #3084, Needs sass/sass-spec#1529

I believe this will fail quite a few spec tests, since it seems we have
some of these as expected outputs!? Lets check what CI returns.

Opened PR to address this in sass-spec.

@mgreter mgreter force-pushed the bugfix/compound-children-order branch 2 times, most recently from 931833c to 8ad5daf Compare May 1, 2020 01:02
@mgreter mgreter self-assigned this May 1, 2020
@mgreter
Copy link
Contributor Author

mgreter commented May 1, 2020

I am going to merge this, as I have some time at hand right now to make a release. Even though knowing it will fail CI, but I cant merge the changes in sass-spec. But I'm certain this is an improvement in the right direction; why not emitting correct css selector when we can. Maybe dart-sass comes up with a different approach than to ensure the order on output.

Note to myself: It might be worthwhile to check if we can integrate id and type selectors directly into the CompoundSelector, since css only allows to have one of each per compound selector. I already did the same for hasRealParent. Could save some allocations and might also simplify selector unifying. We may also just have a dedicated vector for each other type (e.g. class, pseudo, etc). This would completely remove the need to sort the children.

@mgreter mgreter merged commit 49753ba into sass:master May 1, 2020
@ahorek
Copy link
Contributor

ahorek commented May 1, 2020

thanks for the release @mgreter !

@mgreter
Copy link
Contributor Author

mgreter commented May 1, 2020

No problem @ahorek, still quite a few steps to do for a complete release :)
Currently updating my bindings and then the MSI installers ;)

@nex3
Copy link
Contributor

nex3 commented May 2, 2020

This breaks compatibility with Dart Sass and with the specified behavior of Sass. Please roll back for now.

If you want to propose a change to the language itself, you need to follow the language change process.

@xzyfer
Copy link
Contributor

xzyfer commented May 13, 2020

Given the discussion in sass/dart-sass#996 we'll need to revert this PR. As @nex3 this is change to the Sass language and makes us non-compliant. Embedders like Node Sass are now blocked on updating LibSass at the risk of introduce incompatible Sass code into the ecosystem.

@jathak jathak mentioned this pull request May 21, 2020
jathak added a commit to sass/sass-spec that referenced this pull request May 21, 2020
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.

wrong :before position
4 participants