Add support for #rgba and #rrggbbaa #3
Conversation
|
@simonbrunel could you have a look please ? |
| + hexDouble(rgba[1]) | ||
| + hexDouble(rgba[2]) | ||
| + ( | ||
| (a >= 0 && a < 1) |
There was a problem hiding this comment.
I think a === 1 will cause a problem here
There was a problem hiding this comment.
I think it's fine because if a === 1 (opaque), the alpha component is removed from the hex color (#RRGGBB instead of #RRGGBBAA).
There was a problem hiding this comment.
If a === 1, then there is no transparency, so I think we don't need to set the alpha component. I imagine returning only a color formatted as #rrggbb would be a bit faster for the following operations.
There was a problem hiding this comment.
@etimberg What do you think ?
|
@etimberg @kurkle @benmccann would you be able to give a look at that PR? |
| } | ||
| var abbr = /^#([a-fA-F0-9]{3})$/i, | ||
| hex = /^#([a-fA-F0-9]{6})$/i, | ||
| var abbr = /^#([a-fA-F0-9]{3,4})$/i, |
There was a problem hiding this comment.
/^#([a-fA-F0-9]{3,4})$/ or /^#([a-f0-9]{3,4})$/i works here, no need to specify the case insensitivity twice.
There was a problem hiding this comment.
I know, but I think it comes from the original code. Since it doesn't change the result, I would leave it as-is.
There was a problem hiding this comment.
The upstream code has var abbr = /^#([a-f0-9]{3,4})$/i;: https://github.com/Qix-/color-string/blob/master/index.js#L50
| var abbr = /^#([a-fA-F0-9]{3})$/i, | ||
| hex = /^#([a-fA-F0-9]{6})$/i, | ||
| var abbr = /^#([a-fA-F0-9]{3,4})$/i, | ||
| hex = /^#([a-fA-F0-9]{6}([a-fA-F0-9]{2})?)$/i, |
There was a problem hiding this comment.
Same as above. I would remove the A-F part to make this shorter and thus more readable:
/^#([a-f0-9]{6})([a-f0-9]{2})?$/i
There was a problem hiding this comment.
Upstream is var hex = /^#([a-f0-9]{6})([a-f0-9]{2})?$/i;: https://github.com/Qix-/color-string/blob/master/index.js#L51
|
@kurkle I don't think @loicbourgois is still available to update that PR. I think these changes are working correctly so if there is nothing blocking, I would merge as-is. |
|
@simonbrunel what I thought as well and that's why I approved with comments instead of requesting changes. |
|
Sorry, didn't notice the approval :) |
That's because it does not count 😭 😃 |
|
Ah, I didn't check the open PRs before sending mine, so didn't realize there was already one pending. Still I prefer my approach #4 of replacing the function contents with the code from upstream as it's easier to keep in sync with master and would address both open PRs. The other open PR @simonbrunel had suggested #2 (review) copying the code from master as well because it has perhaps a better implementation of what's trying to be accomplished in the other PR |
| assert.equal(string.getAlpha("hwb(244, 100%, 100%)"), 1); | ||
|
|
||
| // alpha | ||
| assert.deepEqual(string.getRgba("#feff"), [255, 238, 255, 1]); |
There was a problem hiding this comment.
it probably would have been preferable to copy the tests from upstream to keep things in sync as much as possible instead of writing our own
5 participants
As discussed in chartjs/Chart.js#5152.
Based on Qix-@88a8282