-
Notifications
You must be signed in to change notification settings - Fork 3
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
Add ContinuousColor
and OrdinalColor
columns
#84
Conversation
8eb711c
to
9d9486f
Compare
Going to relock here, then dig into tests. Ideally, we'd also hit the categorical ones as well. |
ContinuousColor
and OrdinalColor
columns
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.
Working great! No real concerns, only one minor nitpick comment which I'd be happy to have it ignored.
return validate_enum(proposal, ContinuousColor.Scheme) | ||
|
||
|
||
class OrdinalColor(Column): |
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.
This is nitpicking to the n-th degree, so apologies in advance, but should this be CategoricalColor
(or DiscreteColor
)?
My understanding is that ordinal
is for discrete values that have an order (e.g., number of wheels
: [1, 2, 4]
), but categorical
is for those things that don't have an order, e.g., car manufacturer
: ["Honda", "GM", "Ford"]
.
I'm confused by the d3-scale-chromatic
API though, because in their docs they say these scales are Categorical
, but in the code they are under scaleOrdinal
?
Again, apologies for the nitpicking...
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.
at the end of the day, things have to be in an order, both on the input and output side.
we're using that for e.g. domain: ["a",false, 3]
and range: [red, yellow, blue]
would get something, even though the values in either side of the transfer function can't really be compared.
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.
558a12e adds a manual scale example
Checklist
main
for releases, docs-only fixes and post-release hot-fixesdev
for everything elsedoit lint
locallyReferences
Code changes
OrdinalColor
andContinuousColor
Scheme
enum with compatible typesrange
(not really documented)User-facing changes
domain
values have to be known up-front, generallyScreens
Backwards-incompatible changes