-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Ignore colons inside of url(.*) when parsing css #316
Conversation
Parsing by css is totally wrong, but this will at least not break on `url(http://whatever);` anymore! Closes issue cheeriojs#315.
Hi Michael, Thanks for the pull request! This approach address the problem, but it also seems a bit heavy for the problem at hand. If we use What do you say? If you're up for it, could you also add a unit test to |
Thanks for catching this @Meekohi. +1 to @jugglinmike's suggestion. |
@jugglinmike I like it, makes sense as a speed optimization. I'll write a test for it and update this pull request soon. |
@matthewmueller and @jugglinmike this should do the trick and still be very fast. Have a look. |
if (!parts[0]) return obj; | ||
obj[parts[0]] = parts[1]; | ||
var n = str.indexOf(":"); | ||
if(n < 1 || n === str.length-1) return obj; // skip if there is no :, or if it is the first/last character |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you've found (and fixed) another bug! Could you extend your test to verify that we're doing this correctly?
Some code style changes:
- Could you place the comment on the preceding line--we try to stay under 80 characters in the source files.
- Could you insert a space between the
if
keyword and the open parenthesis ((
) character?
Added two more regression tests to make sure parse("color:")=={} and parse(":#ccc")=={}. I think this should be ready to pull in now. |
Merged to |
is still not working for mi case #1223 |
Parsing by css is totally wrong, but this will at least not break on
url(http://whatever);
anymore! Closes issue #315.