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

Separated trimLine content and url regexp's and made them work better #347

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Separated trimLine content and url regexp's and made them work better #347

wants to merge 1 commit into from

Conversation

watzon
Copy link

@watzon watzon commented Sep 30, 2016

Separated the original urlOrCOntentRe into 2 separate regexp's and made them more specific so they don't match incorrect patterns.

@SimenB
Copy link
Owner

SimenB commented Oct 1, 2016

Tests pass, so should be GTG. I don't know enough regex to comment on that part though... @rossPatton?

@@ -1,6 +1,7 @@
'use strict'

var urlOrContentRe = /(["'].+["'])|( +|:)url\(.+\)/
var contentRe = /(content:\s*["'])(?:.+)(["'])/
Copy link
Author

Choose a reason for hiding this comment

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

This regexp creates 2 match groups. The first is the content: " part and the second basically just amounts to the closing quotation mark and possibly semicolon.

@@ -1,6 +1,7 @@
'use strict'

var urlOrContentRe = /(["'].+["'])|( +|:)url\(.+\)/
var contentRe = /(content:\s*["'])(?:.+)(["'])/
var urlRe = /([\S ]+\:\s*url\s*\(["'])(?:.+)(["']\))/
Copy link
Author

Choose a reason for hiding this comment

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

This regexp matches anything before and after a url expression such as background: url("../images/someimage.png") center center; With that expression it would match background: url(" and ") center center;

@SimenB
Copy link
Owner

SimenB commented Oct 5, 2016

@iDev0urer Could you add a test that fails without this change?

I'll do some refactoring in the near future, would be great if this had a regression test in place 😄

Copy link
Owner

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Missing a test or two

@SimenB
Copy link
Owner

SimenB commented Feb 25, 2017

@watzon Ping 😄

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.

None yet

2 participants