Conversation
|
I like that the render code is much easier to read than in #2133; however I think I liked the |
src/mark.d.ts
Outdated
|
|
||
| export interface ColorOptions { | ||
| /** Shorthand for setting both the fill and the stroke. */ | ||
| color?: ChannelValueSpec; |
There was a problem hiding this comment.
This could be supported on all marks…
There was a problem hiding this comment.
Once we support color, we'll often want to use this over stroke or fill, because it's less cognitive work—so it should "just work" everywhere.
I feel it can be a bit tricky though, because it might not make sense for all marks to set both stroke and fill. For instance, I don't think we'd want to set a fill on a line (it's rarely a good idea), or a link (it doesn't apply, so better stick with fill: "none"), but maybe on an arrow (it can apply if there is a non-zero bend), although I would rarely do it.
In this sense "color" should mean "the base color for the mark" rather than "a shorthand for stroke and fill".
There was a problem hiding this comment.
I think the simpler definition of shorthand is easier to understand (and worse is better) and we shouldn’t try to define a “base color” separately for each mark.
I can remove the color shorthand from this PR if you think it’s a bad idea and we can discuss it later.
There was a problem hiding this comment.
Yes, please remove it, I think it might set the wrong expectation
There was a problem hiding this comment.
For what it’s worth we already support a color option on the following marks: axis, grid, auto, crosshair, bollinger. The definition I am using here feels consistent with that existing usage.
Alternative to #2133 and #2405. Fixes #1866 #2111. Needs documentation…