-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add observable10 palette #3843
Conversation
Before we merge, I'd like to get some sign off from other steering committee members. |
I went into the code to double check if its just one pal---it does appear to be, see here---and realized my image-based measurement of the colors was slightly incorrectly, colors should now be right |
Can we pull in d3/d3-scale-chromatic#51 directly? Otherwise we should check for license/attribution requirements. |
Odd question but: would the Observable folks be happy if we were to include the color palette? I'm not sure if color palettes are licensed but, given the name, I wonder if they want their palette to be more readily identifiable with their brand/Plot? I would hate to step on their toes. (The fact that they've contributed it back to D3 makes me think it's probably ok, but it'd be good to get their explicit approval.) |
these are fair points! Elsewhere Maureen also noted that "If you want to add it, let’s please reach out to observable and get both the correct colors and and permission. Lots of work goes into making a good general purpose palette, let’s respect that."
|
Let's just ask @mbostock: Mike, do you (and Observable) support adding your new |
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.
👍👍 Go for it! Thanks for asking.
Please hold on, we're making a slight adjustment to the palette. |
I would prefer to hold off and just import d3/d3-scale-chromatic#51 directly. |
The scheme has been updated (reordered) in plot 0.6.13. Per our CHANGELOG:
The correct scheme (reference below) is identical to d3-scale-chromatic#51. Please, update this PR to match (or follow @domoritz and use d3-scale-chromatic). I want to thank @mcnuttandrew and the team here for your enthusiasm and reactivity. It's by reviewing this PR that I noticed the discrepancy, and our mistake! (And fortunately it was not too late to hot-fix.) (Note the green in position 4.) |
For simplicity and to avoid bloat I would recommend that we not add a new dependency in this PR -- just add the palette within our existing setup. If we want to add a d3-scale-chromatic dependency we should also reconsider our palette system. At the moment (for better or worse) a direct replacement will "break" some of the adjustments in place for Vega color schemes. |
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.
Let's replace with an import so we are up to date with the latest colors.
29863c1
to
f5f6b1e
Compare
Alright with some offline help from @domoritz, I have gotten the scale-chromatic import working as requested. The version of d3-scale-chromatic that is current published (v3.0.0) does not include observable 10, and so i have just updated the stringified colors per Fil's updates. Once the new version is published we can just add a link so we'll always have the most up to date colors. The way i'm including the scale-chromatic schemes is trying to be as un-distruptive as possible, which necessitates a slightly odd hack in which schemes from scale-chromatic are converted from hex[] back into the strings of hexes that generates them. A more holistic rethinking of how palettes work is certainly possible, but in this modestly scoped PR I'm merely trying to make it possible to keep up to date within the current architecture. |
Should we wait for the release then? |
Sorry, just to double check my understanding, wasn't the preference that @jheer mentioned above to not use a d3-scale-chromatic because there are "adjustments in place for Vega color schemes"? |
Ah, I missed that. #3843 (comment). @mcnuttandrew did you make the necessary adjustments? If not, let's just copy. |
Sure! I'll follow up once things d3-scale-chromatic versions
This PR is kinda doing both! The way scale-chromatic is added here does not change the current color support in any way. It totally could be more thoughtfully rethought into something fancier, but for now its just providing a link to the small set of schemes that can be supported without any changes to vega's color support (i can also undo this add, im not very committed to it) |
I would prefer an import over copy but I don't know what the adjustments in place are. |
So, to recap:
However, the colors remain hardcoded in the documentation. I don't expect them to change, but if they do there would be a mismatch between the documentation and the colors actually used. How do you see this? Should we test whether the colors are still as expected? |
packages/vega-scale/src/palettes.js
Outdated
@@ -62,19 +74,22 @@ export const continuous = { | |||
darkRed: '3434347036339e3c38cc4037e75d1eec8620eeab29f0ce32ffeb2c' | |||
}; | |||
|
|||
const toStream = (scheme) => scheme.map((x) => x.slice(1).toLowerCase()).join(''); |
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 be cheaper to lowercase at the end after joining?
Yep @keckelt that is an accurate summary!
I've wondered about the documentation and if it might be better to auto-generate from the palettes, that way there isn't as much manual/error prone work to generate the colors for the docs. I'd be happy to write a script (in a different PR) for generating them if that'd be helpful. (although from first glance i'm not super sure where i'd write such a script as it appears that all of the docs are hard coded?)
I am confident that the colors (as manifest in this PR) are exactly the same. Here's the script I was running inside of the tests to figure out what's what. import * as scaleChromatic from 'd3-scale-chromatic'
Object.keys(discrete).forEach((name) => {
const vegaScheme = discrete[name];
const chromaticName = `scheme${name[0].toUpperCase()}${name.slice(1)}`;
if (!scaleChromatic[chromaticName]) {
return;
}
const chromaticScheme = toStream(scaleChromatic[chromaticName]);
if (vegaScheme === chromaticScheme) {
console.log(name, chromaticName, 'matched!')
}
}); Which returned category10 schemeCategory10 matched!
accent schemeAccent matched!
dark2 schemeDark2 matched!
paired schemePaired matched!
pastel1 schemePastel1 matched!
pastel2 schemePastel2 matched!
set1 schemeSet1 matched!
set2 schemeSet2 matched!
set3 schemeSet3 matched! Something like this could be added as a test to the vega-scale test suite, but it also seems weird to test a dep like that? Happy to add it if you want though. |
Yes, I also find it a bit weird. That was the only thing I noticed when I looked through the PR. That's why I wanted to point it out again. Not critical for me. |
@mcnuttandrew could you wrap up this pull request since there has been a lot of work around it already? |
my understanding is that i was blocked by d3/d3-scale-chromatic#51 landing the new observable 10 |
Should we make in the stream until that's released? I think the scheme doesn't change often if at all. |
I am not entirely convinced that its true that it won't change, which I suspect is why it's idling. In any case, I'm not sure what else needs to be done to wrap it up? I think i addressed the requested changes? but maybe im missing something obvious? |
It will not change, but we need to take a moment to cut a new release (of d3-scale-chromatic but also of its dependents: d3, Plot). |
What's the performance impact of running It's probably tiny given that the method is just const toStream = (scheme) => scheme.map((x) => x.slice(1)).join('').toLowerCase(); |
It appears to be trivial. it's a little hard to test because it is already so fast, but it appears add about 5 milliseconds (measured from within the test suite). (although it may be faster still with out testing) |
Okay. Then let's wait for d3/d3-scale-chromatic#51 and then merge this. |
d3-scale-chromatic 3.1.0 has now been published. |
@domoritz i think this is ready to go now! |
packages/vega-scale/src/palettes.js
Outdated
export const discrete = { | ||
category10: '1f77b4ff7f0e2ca02cd627289467bd8c564be377c27f7f7fbcbd2217becf', | ||
accent: toStream(schemeAccent), | ||
category10: toStream(schemeCategory10), |
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 think it's funny that we are basically reversing the color generation from https://github.com/d3/d3-scale-chromatic/blob/2c52792197299346b7bdb94322bb4dff8f554fea/src/categorical/category10.js#L3.
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.
Small warning here: these are currently hexadecimal colors, but the format might change in the future. I don’t think in practice we will change them, but I don’t want to commit to never changing them. And certainly we might introduce new color schemes in the future that using a different format (e.g., to use the wider p3 color space).
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 think it's funny that we are basically reversing the color generation
this is an irony very much not lost on me :)
Small warning
That's a very good point, but my intuition is that's a bridge that can be crossed when it's encountered. What would be the right way to guard against this change? Including a note saying to look out for that? Include a check in toStream that throws if it encounters not hex characters?
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.
Yeah, we can cross that bridge when we come to it.
For now, I wonder whether we can remove the toStream
method and recognize if we have an array of colors in the palette and skip the colors
method in schemes.js.
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.
ok that appears to be fine
Thank you @mcnuttandrew |
**docs** * remove Google Analytics. (via #3894) (Thanks @domoritz!) **monorepo** * switch to ESLint flat config file. (via #3920) (Thanks @lsh!) * update node versions in ci. (via #3915) (Thanks @domoritz!) **vega-encode** * use domainMin and domainMax to set scale padding. (via #3906) (Thanks @Jami159 & @lsh!) **vega-scale** * Add observable10 palette. (via #3843) (Thanks @mcnuttandrew!) * Respect tickMinStep for binned scale. (via #3904) (Thanks @alexxu-db!) **vega-scenegraph** * Revert TooltipHideEvent to MouseOutEvent to fix events on mobile. (via #3909) (Thanks @kadamwhite!) **vega-typings** * Add observable10 palette. (via #3843) (Thanks @mcnuttandrew!) **vega-view** * turn off all handlers in View.finalize() to fix memory leak. (via #3896) (Thanks @cmerrick!)
Announced here observable has a new color palette called observable10. I don't have a great sense if this type of keeping of with the color-pal-joneses would be seen as useful here, but I figured I'd add it and see what you all thought.