| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
|
||
| 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.
| 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.
There was a problem hiding this comment.
Good catch.
Sorry, something went wrong.
| // 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.
| // 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| }; | ||
|
|
||
| supplyLayoutDefaults(layoutIn, layoutOut, fullData); | ||
| expect(layoutOut.xaxis.gridcolor).toEqual('rgba(255, 247, 0, 0.95)'); |
There was a problem hiding this comment.
Maybe add a comment here stating what gridcolor we would expect if the weight was not scaled by alpha?
Sorry, something went wrong.
There was a problem hiding this comment.
That's reasonable. I'll add it.
Sorry, something went wrong.
| Color.fill({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | ||
|
|
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
| Color.stroke({ style: (o) => seen.push(o) }, undefined); // Mock the selection to track it's call | ||
|
|
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
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.
| const other = Color.equals(picked, Color.background) ? Color.defaultLine : Color.background; | ||
|
|
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| // 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.
| // 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| // 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.
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.
There was a problem hiding this comment.
I'm going to update these to be less focused on alpha.
Sorry, something went wrong.
| }); | ||
| }); | ||
|
|
||
| describe('parse', () => { |
There was a problem hiding this comment.
Honestly all of these parse tests could probably be rolled into other sections
Sorry, something went wrong.
| 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.
| // Null channels used to reach the WebGL buffers through this path. |
Sorry, something went wrong.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| 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)'); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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.
Instead of all this, could you just expect(fills).toBe(...) and copy the actual array expected for the Viridis colorscale?
Sorry, something went wrong.
There was a problem hiding this comment.
Sure, that seems more reasonable.
Sorry, something went wrong.
| * 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.
| * 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| * 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.
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.
There was a problem hiding this comment.
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.
| * Return the alpha channel of a color (0 if falsy). | ||
| * | ||
| * @param {*} cstr - color specifier | ||
| * @return {Number} |
There was a problem hiding this comment.
What is the range for the number, is it [0, 1]?
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, culori clamps to that range.
Sorry, something went wrong.
| * @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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Added.
Sorry, something went wrong.
| /** | ||
| * 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.
| * 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.
There was a problem hiding this comment.
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.
| * @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.
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.
There was a problem hiding this comment.
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.
|
|
||
| /** | ||
| * 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.
| * 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| * @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.
| * @return {String} resulting color string | |
| * @return {String} resulting color string as rgb |
Sorry, something went wrong.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| 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.
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.
There was a problem hiding this comment.
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.
| * 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.
| * 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.
There was a problem hiding this comment.
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.
| /** | ||
| * 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.
| * A missing color paints opaque black, the same as `stroke`. | |
| * A missing or invalid color specifier applies opaque black. |
Sorry, something went wrong.
There was a problem hiding this comment.
I removed the comment in favor of the module note at the top of the file.
Sorry, something went wrong.
| 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.
I'm sure this is fine, but do you know where these equations come from?
Sorry, something went wrong.
There was a problem hiding this comment.
It came from Qix-/color, which got it from SASS. I'll add a note.
Sorry, something went wrong.
| }; | ||
|
|
||
| /** | ||
| * Convert any color specifier to an `rgb(...)` or `rgba(...)` string, |
There was a problem hiding this comment.
Probably all of these functions should specify that passing an invalid color specifier returns black.
Sorry, something went wrong.
There was a problem hiding this comment.
The top level comment will cover these.
Sorry, something went wrong.
| * 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.
| * 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.
There was a problem hiding this comment.
Updated.
Sorry, something went wrong.
| } | ||
| // `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.
Is there any reason not to clip each of the r, g, b values before returning the color object?
Sorry, something went wrong.
There was a problem hiding this comment.
It originally got clipped here, but it was causing some issues. I moved the clipping to where it was needed.
Sorry, something went wrong.
| 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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Left a bunch of comments, but nothing blocking 🌈
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Description
Switch to the culori color processing library.
Closes #7961.
Changes
Screenshots
color_syntax_formats mock results
Testing
Notes