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

pseudos: implement :icontains, case-insensitive version of :contains #34

Merged
merged 1 commit into from
Nov 22, 2015

Conversation

ganeshv
Copy link
Contributor

@ganeshv ganeshv commented Oct 29, 2015

As discussed in issue 33, with tests.

One of the tests fails and needs a corresponding fix in css-what to strip the quotes of icontains data. I have opened a separate PR for that issue.

icontains: function(next, text){
return function icontains(elem){
return next(elem) &&
getText(elem).toLowerCase().indexOf(text.toLowerCase()) >= 0;
Copy link
Owner

Choose a reason for hiding this comment

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

text.toLowerCase() can be called while compiling (can be moved out of the returned function).

@fb55
Copy link
Owner

fb55 commented Oct 30, 2015

One small improvement and this is good to go :)

@ganeshv ganeshv force-pushed the icontains branch 2 times, most recently from 03b5dfb to 459714e Compare October 30, 2015 09:01
@ganeshv
Copy link
Contributor Author

ganeshv commented Oct 30, 2015

Updates: moved the text.toLowerCase() outside and fixed up tabs, rebased and squished it into the same commit.

@@ -0,0 +1,87 @@
var CSSselect = require("../"),
makeDom = require("htmlparser2").parseDOM,
falseFunc = require("boolbase").falseFunc,
Copy link
Owner

Choose a reason for hiding this comment

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

This isn't used.

@fb55
Copy link
Owner

fb55 commented Nov 22, 2015

Whee, sorry, failed to check this again. jshint produced two errors (see comments), do you want to fix them?

@ganeshv
Copy link
Contributor Author

ganeshv commented Nov 22, 2015

OK, fixed the above two, rebased to master, squashed into single commit.

@fb55
Copy link
Owner

fb55 commented Nov 22, 2015

@ganeshv One of your test cases is still failing: AssertionError: 0 == 3. (Have a look at the travis report).

@ganeshv
Copy link
Contributor Author

ganeshv commented Nov 22, 2015

For that test to pass, it needs css-what with my other pull request which you have already merged. Manually copying index.js from css-what/master to node_modules/... does get the test to pass. Let me know how to proceed.

(Sorry for the back and forth, I'm kinda new to the node.js ecosystem. Thanks for your patience.)

@fb55
Copy link
Owner

fb55 commented Nov 22, 2015

Okay, my bad. I've published a new css-what version, let's hope that does the job :)

fb55 added a commit that referenced this pull request Nov 22, 2015
pseudos: implement :icontains, case-insensitive version of :contains
@fb55 fb55 merged commit 7a7c378 into fb55:master Nov 22, 2015
@fb55 fb55 mentioned this pull request Jan 20, 2018
fb55 added a commit that referenced this pull request Oct 21, 2018
pseudos: implement :icontains, case-insensitive version of :contains
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.

2 participants