GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — feat: Switch to culori color library by camdecoster · Pull Request #7962 · plotly/plotly.js Conversation
|
| Before | After |
|---|---|
 |
 |
Testing
- Check CI results
- Review the color_syntax_formats mock and try changing some of the color specifiers to valid/invalid values
- Try entering some of the color specifiers from the mock into an element through browser devtools and see how it compares to the after results
Notes
- This library bumps up the bundle size a bit (+17KB gzipped) but it's worth the added CSS 4 compatibility
- We can mitigate this in the future by switching to ESM for this file (so it can be tree-shaken)
- I removed the
color-normalizedependency in favor of managing that internally
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
|
||
| nodes.each(function (_, i) { | ||
| const { fill } = this.style; | ||
| const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background; | |
| // Ensure text fill color matches either Color.background or Color.defaultLine | |
| expect([Color.background, Color.defaultLine]).toContain(fill); | |
| const other = Color.equals(fill, Color.background) ? Color.defaultLine : Color.background; |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // A translucent axis color is what exercises the rule: the channel weight | ||
| // scales by the alpha difference, so mixing toward an opaque background | ||
| // moves the channels less than a plain interpolation would. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // A translucent axis color is what exercises the rule: the channel weight | |
| // scales by the alpha difference, so mixing toward an opaque background | |
| // moves the channels less than a plain interpolation would. | |
| // axis.gridcolor is determined by mixing the axis color with the paper and plot background colors. | |
| // If the axis color contains an alpha channel, its weight should scale by the alpha value, so that | |
| // more-transparent colors are weighted less. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| }; | ||
|
|
||
| supplyLayoutDefaults(layoutIn, layoutOut, fullData); | ||
| expect(layoutOut.xaxis.gridcolor).toEqual('rgba(255, 247, 0, 0.95)'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe add a comment here stating what gridcolor we would expect if the weight was not scaled by alpha?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's reasonable. I'll add it.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| Color.fill({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Color.fill({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | |
| Color.fill({ style: (o) => seen.push(o) }, undefined); |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this one because I think it's non-obvious as to why the object is being passed in.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| Color.stroke({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Color.stroke({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | |
| Color.stroke({ style: (o) => seen.push(o) }, undefined); |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to leave this one because I think it's non-obvious as to why the object is being passed in.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background; | |
| // Ensure selected color matches either Color.background or Color.defaultLine | |
| expect([Color.background, Color.defaultLine]).toContain(picked); | |
| const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background; |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // Drawing code needs the alpha of a color it is about to paint, which is | ||
| // not the same question `opacity` answers. A color that is simply unset | ||
| // still gets painted, so it resolves to opaque black. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Drawing code needs the alpha of a color it is about to paint, which is | |
| // not the same question `opacity` answers. A color that is simply unset | |
| // still gets painted, so it resolves to opaque black. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| // Drawing code needs the alpha of a color it is about to paint, which is | ||
| // not the same question `opacity` answers. A color that is simply unset | ||
| // still gets painted, so it resolves to opaque black. | ||
| it('treats a missing color as opaque black, without warning', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description implies that the test checks all channels of the color, but it only checks the alpha. Either the description or the test should be updated (I don't think it checks for a warning either, although maybe our test infrastructure does surface warnings).
Same for the next test.
| it('treats a missing color as opaque black, without warning', () => { | |
| it('treats a missing color as opaque', () => { |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to update these to be less focused on alpha.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| }); | ||
| }); | ||
|
|
||
| describe('parse', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly all of these parse tests could probably be rolled into other sections
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| BAD.forEach((v) => expect(Color.isValid(v)).toBe(false)); | ||
| }); | ||
|
|
||
| // Null channels used to reach the WebGL buffers through this path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // Null channels used to reach the WebGL buffers through this path. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| it('sets alpha', () => { | ||
| expect(Color.addOpacity('red', 0.5)).toBe('rgba(255, 0, 0, 0.5)'); | ||
| expect(Color.addOpacity('rgba(255, 0, 0, 0.5)', 1)).toBe('rgb(255, 0, 0)'); | ||
| expect(Color.addOpacity('red', 2)).toBe('rgb(255, 0, 0)'); |
emilykl
Aug 14, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No opacity is added here: is that because 2 is an invalid value so the function does nothing, or because a ceiling is applied so that 2 becomes 1?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, alpha is clamped to the [0, 1] range. Since 2 is greater than 1, 1 is passed into the formatter. culori drops alpha in this case, so rgb is returned.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| expect(fills.length).toBe(5); | ||
| expect(fills.every((f) => f === 'rgb(0, 0, 0)')).toBe(false, 'all points black'); | ||
| expect(new Set(fills).size).toBeGreaterThan(1, 'every point the same color'); | ||
| fills.forEach((f) => expect(f).toMatch(/^rgba?\(/, `not a usable color: ${f}`)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of all this, could you just expect(fills).toBe(...) and copy the actual array expected for the Viridis colorscale?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that seems more reasonable.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| * Parse a color specifier, falling back to opaque black. | ||
| * | ||
| * A missing color falls back quietly, because it means the caller left the | ||
| * attribute unset rather than gave a bad value. Callers that treat a missing | ||
| * color as nothing to paint test for it themselves, as `opacity` does. | ||
| * | ||
| * @param {*} cstr - color specifier | ||
| * @return {Color} color object | ||
| * @param {Boolean} [silent] - skip the warning, for callers that run per data point |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Parse a color specifier, falling back to opaque black. | |
| * | |
| * A missing color falls back quietly, because it means the caller left the | |
| * attribute unset rather than gave a bad value. Callers that treat a missing | |
| * color as nothing to paint test for it themselves, as `opacity` does. | |
| * | |
| * @param {*} cstr - color specifier | |
| * @return {Color} color object | |
| * @param {Boolean} [silent] - skip the warning, for callers that run per data point | |
| * Parse a color specifier string and return it as a culori rgb color object. | |
| * If the input is not a string or cannot be parsed, fall back to opaque black (#fff). | |
| * | |
| * @param {String} cstr - color specifier | |
| * @param {Boolean} [silent] - if true, do not emit a warning for un-parseable colors |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| * Convert any color specifier to a normalized `rgb(r, g, b)` string. | ||
| * Force alpha to 1 so that it gets dropped in the result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this function calls parse(), that means any invalid input will return rgb(0, 0, 0), right?
Might be worth putting that info in the docstring.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair point, but I think it makes more sense to leave that note in the docstring for parse. It seems better to keep it there since so many functions call parse.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| * Return the alpha channel of a color (0 if falsy). | ||
| * | ||
| * @param {*} cstr - color specifier | ||
| * @return {Number} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the range for the number, is it [0, 1]?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, culori clamps to that range.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| * @param {*} cstr - color specifier | ||
| * @return {Color} color object | ||
| * @param {Boolean} [silent] - skip the warning, for callers that run per data point | ||
| * @return {Object} culori rgb color |
emilykl
Aug 14, 2026
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A culori rgb object is just an object that looks like
{ mode: 'rgb', r: _, g: _, b: _, alpha: _ }
right? Maybe add that info to the docstring since parse() is used everywhere.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| /** | ||
| * Convert a color specifier to a 4-element `[r, g, b, a]` representation. | ||
| * Accepts strings, numeric float arrays (`[0, 1]`), or uint8 arrays (`[0, 255]`). | ||
| * Falls back to opaque black rather than null: WebGL paths index the result. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Falls back to opaque black rather than null: WebGL paths index the result. | |
| * Returns opaque black ([0, 0, 0, 0]) if color specifier is invalid. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to add a note at the top of the file and remove this one because most of the functions would get the same note.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| * @param {*} cstr - color specifier | ||
| * @param {Number} op - opacity in [0, 1] | ||
| * @return {String} | ||
| * @param {Number} op - opacity in [0, 1], clipped to that range |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clipped to that range
Unclear — does this mean the input must be clipped to [0, 1] before passing to this function, or that if it's outside [0, 1] this function will clip it?
In any case, I see the clipping for values above 1, but what happens when the value is below 0?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll update the description to be clearer. If the value is below zero, it gets set to zero. I'm going to rename the function to clamp01.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
|
|
||
| /** | ||
| * Combine two colors into one apparent color by compositing `front` over `back`. | ||
| * If `back` is missing or transparent, the module `background` is assumed behind it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * If `back` is missing or transparent, the module `background` is assumed behind it. | |
| * If `back` is missing, the module `background` is assumed behind it. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| * @param {*} cstr - color specifier | ||
| * @param {Number} delta - lightness shift in HSL percentage points | ||
| * @return {Color} adjusted color object | ||
| * @return {String} resulting color string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * @return {String} resulting color string | |
| * @return {String} resulting color string as rgb |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| const newColor = isDark(cstr) | ||
| ? lightAmount | ||
| ? adjustLightness(c, lightAmount) | ||
| : color(background) | ||
| ? adjustLightness(cstr, lightAmount) | ||
| : background | ||
| : darkAmount | ||
| ? adjustLightness(c, -darkAmount) | ||
| : color(defaultLine); | ||
| ? adjustLightness(cstr, -darkAmount) | ||
| : defaultLine; | ||
|
|
||
| return newColor.rgb().string(); | ||
| return formatRgb(parse(newColor)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If newColor is the result of adjustLightness(), then it's already a formatted rgb string and the extra round-trip through formatRgb(parse(...)) is unnecessary, right?
| const newColor = isDark(cstr) | |
| ? lightAmount | |
| ? adjustLightness(c, lightAmount) | |
| : color(background) | |
| ? adjustLightness(cstr, lightAmount) | |
| : background | |
| : darkAmount | |
| ? adjustLightness(c, -darkAmount) | |
| : color(defaultLine); | |
| ? adjustLightness(cstr, -darkAmount) | |
| : defaultLine; | |
| return newColor.rgb().string(); | |
| return formatRgb(parse(newColor)); | |
| const newColor = isDark(cstr) | |
| ? lightAmount | |
| ? adjustLightness(cstr, lightAmount) | |
| : formatRgb(parse(background)); | |
| : darkAmount | |
| ? adjustLightness(cstr, -darkAmount) | |
| : formatRgb(parse(defaultLine)); | |
| return newColor; |
Honestly it would probably make sense to convert the constants to RGB just once at the top of the file and reuse them.
const backgroundRGB = formatRgb(parse(background)) const defaultLineRGB = formatRgb(parse(defaultLine))
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like your first suggestion (and I'm going to get rid of the nested ternary). I'll add the constants as well. This is the only place where backgroundRGB/defaultLineRGB would get used, but they'll get used a lot so we might as well save the function calls.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| * A missing color paints opaque black. Shapes and annotations leave | ||
| * `line.color` unset when the user gives none, and the outline still has to | ||
| * show. Use `opacity` instead when a missing color means "nothing to paint". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * A missing color paints opaque black. Shapes and annotations leave | |
| * `line.color` unset when the user gives none, and the outline still has to | |
| * show. Use `opacity` instead when a missing color means "nothing to paint". | |
| * A missing or invalid color specifier applies opaque black. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What you deleted is out of date, so I removed it. I also removed the note about black in favor of the top level comment.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| /** | ||
| * Apply `fill` and `fill-opacity` styles to a D3 selection. | ||
| * | ||
| * A missing color paints opaque black, the same as `stroke`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * A missing color paints opaque black, the same as `stroke`. | |
| * A missing or invalid color specifier applies opaque black. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the comment in favor of the module note at the top of the file.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| const w = 2 * p - 1; | ||
| const w2 = ((w * d === -1 ? w : (w + d) / (1 + w * d)) + 1) / 2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure this is fine, but do you know where these equations come from?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It came from Qix-/color, which got it from SASS. I'll add a note.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| }; | ||
|
|
||
| /** | ||
| * Convert any color specifier to an `rgb(...)` or `rgba(...)` string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably all of these functions should specify that passing an invalid color specifier returns black.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The top level comment will cover these.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| * Channels as `[r, g, b, a]`, with `r`/`g`/`b` in [0, 255] and `a` in [0, 1]. | ||
| * An array rather than an object so callers cannot depend on the color library's | ||
| * shape. Unrounded, since callers do further arithmetic. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| * Channels as `[r, g, b, a]`, with `r`/`g`/`b` in [0, 255] and `a` in [0, 1]. | |
| * An array rather than an object so callers cannot depend on the color library's | |
| * shape. Unrounded, since callers do further arithmetic. | |
| * Returns the given color specifier as an `[r, g, b, a]` array, | |
| * with `r`/`g`/`b` in [0, 255] and `a` in [0, 1]. |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
| } | ||
| // `toRgb` omits alpha when it's 1; make sure it's added since we expect it | ||
| c.alpha ??= 1; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any reason not to clip each of the r, g, b values before returning the color object?
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It originally got clipped here, but it was causing some issues. I moved the clipping to where it was needed.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
| var colorOut = rgba(colorIn); | ||
| // A per-point color may be raw channels rather than a color string, which | ||
| // `Color.isValid` rejects but `Color.normalize` handles. | ||
| if (!isArrayOrTypedArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to use Color.isChannelArray() here?
| if (!isArrayOrTypedArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba; | |
| if (!Color.isChannelArray(colorIn) && !Color.isValid(colorIn)) return colorDfltRgba; |
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But actually this logic feels like a code smell. How about adding a parameter arrayAllowed to Color.isValid() so that you could write
if (!Color.isValid(colorIn, true)) return colorDfltRgba;
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep isValid specific to color strings because it gets used in the coerce test. I'll use your first suggestion. validateColor is essentially a wrapper that does what you're suggesting we add to isValid. If this becomes a more common pattern, we can update isValid at that time.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
-
👍 1 reaction
emilykl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a bunch of comments, but nothing blocking 🌈
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
1 more reviewer
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assignedLabels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.