-
Notifications
You must be signed in to change notification settings - Fork 185
clip: true clips the mark to the frame #752
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
Conversation
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.
Good work! Thanks for doing this.
To reduce duplication, I think we can either (1) fold the call to applyClip into applyIndirectStyles (this would require passing dimensions to applyIndirectStyles) or (2) move the clipping logic up to plot.js so that it is part of the base Mark functionality (and the behavior cannot be overridden or ignored by Mark subclasses).
Can we .call(applyIndirectStyles, dimensions) from the main Mark class? Only Plot.text was acting a bit different, but now it's standardized (I unwrapped applyIndirectStyles from applyIndirectTextStyles). |
Interesting question. It sounds tempting, but it would also make it harder for a Mark to change the behavior. I think I’d probably leave it as-is for, mostly because applyIndirectStyles needs to be used together with applyDirectStyles, and if Mark can only take care of applyIndirectStyles I’m not sure it make sense to split them. But I could definitely see us revisiting this question in the future. |
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.
A few minor suggestions.
I would also like to reformat the traffic-horizon.js example (no spaces within braces, and reordering the Plot.plot options to match the convention).
Also, it’d probably make sense to sort the data once outside the Plot rather than using the Plot.sort operator on each area mark? But either is fine.
test/plots/traffic-horizon.js
Outdated
export default async function() { | ||
const data = await d3.csv("data/traffic.csv", d3.autoType); | ||
const bands = 7; | ||
const step = +(d3.max(data, d => d.value) / bands).toPrecision(2); |
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.
You could use d3.tickStep here (or d3.tickIncrement if you want to be really fancy).
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.
Not sure how? With 7 bands and a maximum value = 9616, the step is rounded to 1400, but d3.ticks(0, max, bands + 1) returns 9 bands (i.e. 10 ticks = [0, 1000, 2000, …, 9000]).
Besides the technical point, I like the original example which clearly shows (with the select for the number of bands) how horizon charts are built.
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.
In my PR #753 you can say:
const bands = 7; // just a hint; not guaranteed
const max = d3.max(data, d => d.value);
const step = d3.tickStep(0, max, bands);
const ticks = d3.ticks(0, max, bands);
This results in 10 bands spaced every 1,000:
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.
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'm going to merge the 5 bands with the lower bound.
test/plots/traffic-horizon.js
Outdated
fy: { axis: null, domain: new Set(data.map((d) => d.name)) }, | ||
x: { axis: "top" }, | ||
y: { axis: null, domain: [0, step] }, | ||
color: { scheme: "blues", range: [0.05, 1], type: "ordinal", legend: true }, |
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.
Feels like a bit of a bummer than the range is needed here, like these color schemes don’t work out of the box? Is there something about this application that makes the ColorBrewer schemes to light, compared to say a choropleth?
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.
Just a tiny aesthetic preference I had in the notebook—we can remove it.
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 remember now why I played with this: the thing is that the first band represents values between 0 and 1 * step, and the paler shade of blue should represent 0; here what I'd really want is the color representing step/2. I'll make this more explicit.
Co-authored-by: Mike Bostock <[email protected]>
Co-authored-by: Mike Bostock <[email protected]>
* tweak horizon example * use offset, not index
closes #165
demo https://observablehq.com/@observablehq/plot-horizon-chart
this is just a first step, which does not address #181 where an individual mark (a text) needs to be clipped by a data-driven clip-path.