-
Notifications
You must be signed in to change notification settings - Fork 142
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
Brush & zoom #222
Comments
So, I just tried porting to D3 6 the “Brush and Zoom” example that this codepen is based on (gist). It appears that there is a bug in the sense that, to avoid looping between the brush and zoom events, we test the event.sourceEvent. With the global d3.event this did not matter since it was "always there". However, now that the event is local, when we call brush.move and zoom.transform directly we do not pass it, and the test always fails. I see two possibilities: 1) Use a different method to avoid looping.For exemple here I create a global zoomevent to track the double calls (note that it could alternatively be a property of a DOM element if we are sure that zoom and brush are applied to the same element). + let zoombrush;
- function brushed() {
- const event = d3.event;
- if (event.sourceEvent && event.sourceEvent.type === "zoom") return; // ignore brush-by-zoom
+ function brushed(event) {
+ if (zoombrush) return; // ignore brush-by-zoom
+ zoombrush = 1;
var s = event.selection || x2.range();
x.domain(s.map(x2.invert, x2));
focus.select(".area").attr("d", area);
focus.select(".axis--x").call(xAxis);
svg.select(".zoom").call(zoom.transform, d3.zoomIdentity.scale(width / (s[1] - s[0])).translate(-s[0], 0));
+ zoombrush = 0;
}
- function zoomed() {
- const event = d3.event;
- if (event.sourceEvent && event.sourceEvent.type === "brush") return; // ignore zoom-by-brush
+ function zoomed(event) {
+ if (zoombrush) return; // ignore zoom-by-brush
+ zoombrush = 1;
var t = event.transform;
x.domain(t.rescaleX(x2).domain());
focus.select(".area").attr("d", area);
focus.select(".axis--x").call(xAxis);
context.select(".brush").call(brush.move, x.range().map(t.invertX, t));
+ zoombrush = 0;
} This method can be applied today "in user land". See the new example in https://blockbuilder.org/Fil/076415313a08e76ae60d468e2cb25909 2) pass the event to brush.move and to zoom.transform.(This seems more complex and needs to patch brush.move) - svg.select(".zoom").call(zoom.transform, transform);
+ svg.select(".zoom").call(zoom.transform, transform, null, event); - context.select(".brush").call(brush.move, x.range().map(t.invertX, t));
+ context.select(".brush").call(brush.move, x.range().map(t.invertX, t), event); This leaves the user code closer to the original, but it can not work as of today, because brush.move does not currently accept an event. Maybe we should allow it and patch it like so:
|
@Fil Thanks for your help here! Your example seems to work flawlessly. Also I strongly guess my code was based on that example initially (I didn't write it). However I tried applying your user land fix in the app and I'm still seeing the issue. My CodePen reproduction doesn't even use the On the other hand my real code has the I'm still investigating what is different in your example so that it works … |
Ah yes—when I was exploring your codepen I changed
to
so that the zoom behavior would not fire on the context part. That's when I saw that there was no |
Oh right, that avoids the issue. Thank you very much for your help! |
Yes. We need to research the issue in itself, which might be a regression—but I'm not sure yet if it's linked to #198. |
This seems to: - fix #83 - possibly fix d3/d3-zoom#198 - help with d3/d3-zoom#222
This seems to: - fix #83 - possibly fix d3/d3-zoom#198 - help with d3/d3-zoom#222
* create emit before we use it; make move accept an event. This seems to: - fix #83 - possibly fix d3/d3-zoom#198 - help with d3/d3-zoom#222 * update dependencies * brush.clear event Co-authored-by: Mike Bostock <mbostock@gmail.com>
Seems zoom and brush still steal each other's events (version 7.4.4). I have found different solutions how to use zoom and brush together. But all of them looks like workarounds or hacks. Here a small example: https://codepen.io/buharov-alexander/pen/mdXwvdY |
This particular issue has been resolved. @buharov-alexander please can you open a new discussion instead, with a precise use-case? Note that you should be able to avoid the brush or zoom behavior "stealing" events by using brush.filter or zoom.filter. |
bootstrap to 4.6.2 jquery to 3.7.0 jquery-ui to 1.13.2 d3 to v7 moment.js to 2.29.4 Some d3 changes were required: 1. The global d3.event was replaced by events passed into each handler. See https://observablehq.com/@d3/d3v6-migration-guide#event_brush. 2. Item 1 had an effect related to the brush and zoom handlers. See the comment added in the code and this issue: d3/d3-zoom#222. One Bootstrap change was required: button styling changed and the buttons had no background color. Added the btn-light class to the buttons.
From @amannn (copied from #198 (comment) ):
I'm having issues with the `mouseup` event not being handled correctly.There's a zoom applied to the chart and when I'm dragging in the context area below, the
mouseup
doesn't seem to end the pan gesture. Themousemove
events that happen aftermouseup
cause the chart to be panned again. Once I pan the regular chart above, the panning stops correctly.This behaviour happened after an upgrade to
d3-zoom@2.0.0
.Here's a reproduction: https://codepen.io/amann/pen/QWNqdqw?editors=1010. Sorry that the example is a bit convoluted. I'm new to d3 and wasn't sure which parts I could safely remove.
The text was updated successfully, but these errors were encountered: