Skip to content
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

explicit colors #660

Closed
wants to merge 3 commits into from
Closed

explicit colors #660

wants to merge 3 commits into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Jan 10, 2022

if all the categories are valid CSS colors and no range or scheme has been set, the scale type defaults to identity.

Ref: https://talk.observablehq.com/t/specific-coloring-for-bar-plots/5996

Note that with this PR, you can still be surprised by "wrong colors": if any of the channels has an undefined value, the colors will switch back to tableau10. The user needs to use explicit colors everywhere for it to work.

I filed a commit with unit tests and another one with the implementation, because I initially had an alternative implementation where a domain comprised only of valid CSS colors would be promoted to the default range of a categorical scale.

Fil added 3 commits January 10, 2022 08:44
…een set, the scale type defaults to identity (tests)
…een set, the scale type defaults to identity (implementation)
@Fil Fil requested a review from mbostock January 10, 2022 07:53
@Fil Fil changed the title default color type = identity when all values are valid CSS colors explicit colors Jan 10, 2022
@mbostock
Copy link
Member

I need to think about this one. One question is whether it should behave more similarly to how we disambiguate temporal vs. quantitative vs. ordinal. Like, perhaps the test should be based on the first non-null channel value? If that’s a valid CSS color, then we use an identity scale (and we don’t test the rest of the values). That feels like it might be more consistent with our other behavior, but then it also feels more error prone since you could have a false positive (mostly with the named colors). And if there is going to be ambiguity, maybe it’s good to require an explicit declaration of the identity scale. I don’t think it’s necessarily a bad thing that people get confused here if it results in them reconfiguring their understanding of Plot. Anyway… like I said I want to think on this a bit.

@Fil
Copy link
Contributor Author

Fil commented Jan 12, 2022

Yes. As an alternative I thought we could build the scale as keeping the valid colors from the domain, but taking unknown from tableau10. So that if a category is "red" and another one "rabbit", the red category gets the red color, and the rabbit category gets #123456 from tableau10. It doesn't allow the user to return undefined to remove a mark (for this they will need to specify "identity"), but it seems to cover the other cases.

@mbostock
Copy link
Member

Closing in favor of #673. 🙏

@mbostock mbostock closed this Jan 17, 2022
@Fil Fil deleted the fil/default-category-color branch January 17, 2022 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants