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

add ":not" to negate css prop selector #460

Closed
wants to merge 2 commits into from
Closed

add ":not" to negate css prop selector #460

wants to merge 2 commits into from

Conversation

arch-mage
Copy link

this change based on this issue.

P.S: I've never done any pull request to any project before. This is my first time, please tell me if I did something wrong or I miss anything important.

@aweary
Copy link
Collaborator

aweary commented Jun 16, 2016

@arch-mage great work on your first PR, we really appreciate it! This is going to be affected by #458, which totally reimplements how we parse selectors, so I'm not sure if we want to merge this if it's not going to be compatible.

@ljharb
Copy link
Member

ljharb commented Jun 16, 2016

Personally I like the idea of merging this first, and then making sure #458 still passes the new tests after a rebase :-)

@aweary
Copy link
Collaborator

aweary commented Jun 16, 2016

That's fine with me :) @arch-mage can we add some more tests to verify that this works with other selectors like classes and ids?

':not(.foo)'
':not(#bar)'

I think ideally we'd have a negation test for every query combination/type we support. We've had a number of issues arise from corner cases with our regex parsers.

@arch-mage
Copy link
Author

arch-mage commented Jun 16, 2016

I think it's better to wait for #458 first. Because, when I wrote this, i only think about negating attribute. So it won't work on :not(#foo) or :not(.bar). It's kind a little hacking I think, which obviously will break. I will see then, if I can do adding :not selector

@ljharb
Copy link
Member

ljharb commented Sep 26, 2017

@aweary is this still needed in v3?

@aweary
Copy link
Collaborator

aweary commented Sep 26, 2017

@ljharb it's still needed, but this PR is now totally outdated. I'll be sending a PR for this today 😃 @arch-mage thanks for your work on this, sorry we didn't get it merged!

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