Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

made whitespace character optional #8057

Merged
merged 5 commits into from
Jun 16, 2014
Merged

made whitespace character optional #8057

merged 5 commits into from
Jun 16, 2014

Conversation

h1w4y
Copy link
Contributor

@h1w4y h1w4y commented Jun 7, 2014

Addresses #8028 - Quick View doesn't recognize linear-gradient if no space before it

Updated regex to make whitespace character optional before "linear" or "radial"

// Quick View Always Worked for this
background : linear-gradient(to bottom, black 0%, white 100%);
background : radial-gradient(to bottom, black 0%, white 100%);
background: linear-gradient(to bottom, black 0%, white 100%);
background: radial-gradient(to bottom, black 0%, white 100%);

// Now this Works Too
background :linear-gradient(to bottom, black 0%, white 100%);
background :radial-gradient(to bottom, black 0%, white 100%);

@redmunds redmunds self-assigned this Jun 10, 2014
@redmunds
Copy link
Contributor

@HighwayChile This works great. Can you add 2 unit tests: 1 for linear-gradient and 1 for radial-gradient?

@h1w4y
Copy link
Contributor Author

h1w4y commented Jun 10, 2014

@redmunds Yes! Perfect opportunity -- I'm just starting to look at how brackets tests are organized.

@redmunds
Copy link
Contributor

Excellent -- let me know if you have any questions.

@@ -162,7 +162,7 @@ define(function (require, exports, module) {

// Check for gradient. -webkit-gradient() can have parens in parameters
// nested 2 levels. Other gradients can only nest 1 level.
var gradientRegEx = /-webkit-gradient\((?:[^\(]*?(?:\((?:[^\(]*?(?:\([^\)]*?\))*?)*?\))*?)*?\)|(?:(?:-moz-|-ms-|-o-|-webkit-|\s)((repeating-)?linear-gradient)|(?:-moz-|-ms-|-o-|-webkit-|\s)((repeating-)?radial-gradient))(\((?:[^\)]*?(?:\([^\)]*?\))*?)*?\))/gi,
var gradientRegEx = /-webkit-gradient\((?:[^\(]*?(?:\((?:[^\(]*?(?:\([^\)]*?\))*?)*?\))*?)*?\)|(?:(?:-moz-|-ms-|-o-|-webkit-|\s?)((repeating-)?linear-gradient)|(?:-moz-|-ms-|-o-|-webkit-|\s?)((repeating-)?radial-gradient))(\((?:[^\)]*?(?:\([^\)]*?\))*?)*?\))/gi,
Copy link
Contributor

Choose a reason for hiding this comment

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

@HighwayChile After thinking about this a bit more, instead of making preceding whitespace optional, I think we should add option for a colon. This will eliminate false positives for something like xyz-linear-gradient(...). Will that work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@redmunds hmm, let me look at the regex more closely.

@h1w4y
Copy link
Contributor Author

h1w4y commented Jun 10, 2014

@redmunds after looking at the regex more, your suggestion is more readable and better matching. Will try to get some tests in tomorrow. Woohoo!

@h1w4y
Copy link
Contributor Author

h1w4y commented Jun 13, 2014

@redmunds ok tests are in

.whitespaceCheck_linear_gradient {
background : linear-gradient(to bottom, black 0%, white 100%);
background: linear-gradient(to bottom, black 0%, white 100%);
background :linear-gradient(to bottom, black 0%, white 100%);
Copy link
Contributor

Choose a reason for hiding this comment

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

The most common CSS whitespace style between property and value is prop: val, and the second most popular is no whitespace at all: prop:val so add that case (both here and for radial-gradient).

@redmunds
Copy link
Contributor

@HighwayChile Just 2 more minor comments.

@h1w4y
Copy link
Contributor Author

h1w4y commented Jun 15, 2014

@redmunds I used a percentage for the radial values because I got an odd result when using the px values. It looks like when the test.css is read for testing, the px value is interpreted as a percentage value -- then it fails because the test is expecting the px value.

Error: Expected 'radial-gradient(red, white 50%, blue 100%)' to be 'radial-gradient(red, white 20px, blue 40px)'.

I can look into this more if it's helpful. Otherwise, I'll stick with the percentage values.

@redmunds
Copy link
Contributor

Look good. Thanks. Merging.

redmunds added a commit that referenced this pull request Jun 16, 2014
@redmunds redmunds merged commit 138e963 into adobe:master Jun 16, 2014
@h1w4y
Copy link
Contributor Author

h1w4y commented Jun 17, 2014

@redmunds Awesome, Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants