Skip to content

Conversation

@mbostock
Copy link
Member

@mbostock mbostock commented Jan 20, 2022

We do this for number and date, so we should for boolean, too. Previously #346.

@mbostock mbostock requested a review from Fil January 20, 2022 17:56
@Fil
Copy link
Contributor

Fil commented Jan 20, 2022

Agree that it's better than to filter them out.

But it's a little bit surprising at least in the following case:

Plot.dot(data, {x…, fill: true})

Reading this code, you might imagine that fill: true means you want the dots to be filled, and fill: false that you prefer the default stroked shapes. Conversely for bars, you might thin that fill: false would reverse the default.

Instead what this does is give them a category which makes them filled blue — in both cases: true and false appear filled blue, at least if there's nothing else that drives more colors.

@mbostock
Copy link
Member Author

mbostock commented Jan 20, 2022

Plot.dot(data, {x…, fill: true})

It’s equivalent to this, though, right?

Plot.dot(data, {x…, fill: () => true})

And when that’s defined by data…

Plot.dot(data, {x…, fill: d => d.likesCheese})

You almost certainly do want the categorical color encoding. So, I think this makes sense for consistency.

An alternative would be, like we do for the identity color scale, if all the associated defined values are booleans, we could map that to “currentColor” and “none” or some such. But then the false values would typically not be visible at all, unless you also had a stroke. Do you prefer that?

@Fil
Copy link
Contributor

Fil commented Jan 20, 2022

It's logical in all the cases to have fill: true create filled dots. The fact that they appear to be blue is secondary (and could be fixed by defaulting the ordinal scale to ["currentColor"] when the domain is [true]). The surprising bit is that fill: false makes them blue too.

What if, when the domain is comprised only of booleans+undefined, we would map true to "currentColor" and false to maybe a middle grey #888—and undefined to invalid? Then I think it would do the expected thing (at least "expected in my mind", I reckon that there are several ways to think about this).

@mbostock
Copy link
Member Author

I did roughly that in #693, but mapping false to “none”. Wouldn’t saying fill: false and getting a (non-none) fill be confusing?

@mbostock
Copy link
Member Author

Another possibility is that we could special-case the default scheme if the associated domain is entirely booleans. So, it’d still be an ordinal scale, but you’d set dark/light gray instead of tableau10. I’ll give that a shot…

@Fil
Copy link
Contributor

Fil commented Jan 20, 2022

This last option (special case a boolean-only ordinal scale to two greys) has my vote.

@Fil Fil mentioned this pull request Jan 20, 2022
Copy link
Contributor

@Fil Fil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great in combination with #694

@mbostock mbostock merged commit 536dec8 into main Jan 20, 2022
@mbostock mbostock deleted the mbostock/boolean-valueof branch January 20, 2022 21:31
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