Conversation
mdoliner
left a comment
mdoliner
left a comment
There was a problem hiding this comment.
Sorry for the delay - we didn't see this come in! Thanks for contributing to jquery-expect!
| 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
| 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
|
|
3 participants
Having spaces in hex codes breaks css assertions like
$expect('h2').to.have.css('color', 'blue');