remove spaces from color hex codes #18
Conversation
| @@ -460,11 +460,11 @@ | |||
| */ | |||
|
|
|||
| function rgb2hex (rgb) { | |||
| rgb = rgb.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/); | |||
| return "#" + | |||
| rgb = rgb.match(/^rgba?[\s+]?\([\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?/i); | |||
There was a problem hiding this comment.
rgba to hex would be a little more complicated - we shouldn't accept 'a' here since it won't correctly convert the color - it would be better to create a separate function rgba2hex with the correct logic instead of trying to overload this one
| @@ -460,11 +460,11 @@ | |||
| */ | |||
|
|
|||
| function rgb2hex (rgb) { | |||
| rgb = rgb.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/); | |||
| return "#" + | |||
| rgb = rgb.match(/^rgba?[\s+]?\([\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?/i); | |||
There was a problem hiding this comment.
I believe a more concise way to accomplish what you are trying to accomplish with [\s+]? would be \s*
| rgb = rgb.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/); | ||
| return "#" + | ||
| rgb = rgb.match(/^rgba?[\s+]?\([\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?/i); | ||
| return (rgb && rgb.length === 4) ? "#" + |
There was a problem hiding this comment.
By nature of there being 3 capturing groups, a match will always have a length of 4 - can you elaborate on what is added by this check
| rgb = rgb.match(/^rgb\((\d+),\s*(\d+),\s*(\d+)\)$/); | ||
| return "#" + | ||
| rgb = rgb.match(/^rgba?[\s+]?\([\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?,[\s+]?(\d+)[\s+]?/i); | ||
| return (rgb && rgb.length === 4) ? "#" + |
There was a problem hiding this comment.
Putting error handling in for a non-match I think is a good idea - however I don't think it should silently fail like this - feel free to add a more explicit error to be thrown than the default, but I don't think returning an empty string should be expected functionality
|
|
Having spaces in hex codes breaks css assertions like
$expect('h2').to.have.css('color', 'blue');