-
Notifications
You must be signed in to change notification settings - Fork 187
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
document layouts #713
document layouts #713
Conversation
note: the custom layout example will change a little if we do #712
README.md
Outdated
layout: (index, scales, values, dimension) => { | ||
if (values.fill) { | ||
for (const i of index) { | ||
values.fill[i] = d3.rgb(values.fill[i]).darker(); |
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.
We shouldn’t use/recommend mutation.
Co-authored-by: Mike Bostock <[email protected]>
README.md
Outdated
```js | ||
Plot.dot(data, { | ||
layout: (index, scales, values, dimension) => { | ||
if (values.fill) { |
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 example does nothing out of the box, because values.fill will always be undefined. Perhaps a more realistic example would be better? For example, maybe you could have a helper function…
function darker(key = "fill", amount = 1) {
return (index, scales, {fill, ...values}, dimension) => {
fill = fill.slice();
for (const i of index) fill[i] = d3.rgb(fill[i]).darker(amount);
return {...values, fill};
};
}
And then
Plot.dot(penguins, {fill: "body_mass_g", layout: darker()})
I guess even this feels very contrived, since it’d be better to do this on the scale definition. Maybe we should remove the custom layouts section until we can think of a better motivating example?
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.
layout: contrasting on Plot.text could be short (and fix #540). A good contrasting color not as simple as checking a color's lightness, though, so an official implementation would likely diverge from such an example.
Removing this part for now.
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.
Choosing a contrasting color sounds interesting and useful. Perhaps the fill input channel specifies the background color (e.g., fill: "imdb_rating"
in the Simpsons example), and then the layout outputs a new fill channel where each output color is either light or dark depending on the lightness of the input? Something like:
Plot.text(simpsons, Plot.contrast({
x: "season",
y: "number_in_season",
text: d => d.imdb_rating?.toFixed(1),
title: "title",
fill: "imdb_rating"
}))
Though, some sort of magic like this might be better…
Plot.text(simpsons, {
x: "season",
y: "number_in_season",
text: d => d.imdb_rating?.toFixed(1),
title: "title",
fill: Plot.contrast("imdb_rating")
})
Which, I guess, would be like a per-channel layout.
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'll open a new issue and prototype :)
Superseded by #775. |
note: the custom layout example will change a little if we do #712