Skip to content
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

Two #71

Merged
merged 15 commits into from
Aug 23, 2020
Merged

Two #71

merged 15 commits into from
Aug 23, 2020

Conversation

Fil
Copy link
Member

@Fil Fil commented May 15, 2020

Fil added 2 commits May 15, 2020 15:15
(rationale: don't let people install @2 in a build system that will not alert them that we have moved to ES6, only to cause trouble with a later release.)
src/brush.js Outdated Show resolved Hide resolved
@Fil Fil marked this pull request as ready for review May 20, 2020 12:54
@Fil Fil marked this pull request as draft May 22, 2020 14:43
Fil added 4 commits May 22, 2020 17:25
… the new gesture gets overridden

this showed in the brush transition notebook https://observablehq.com/d/a5be418e6d408119 where interrupting the "brush.move" after a first gesture would not emit the proper sourceEvent in the subsequent brushend
@Fil Fil marked this pull request as ready for review July 2, 2020 14:21
@Fil
Copy link
Member Author

Fil commented Jul 2, 2020

New version!

src/brush.js Outdated Show resolved Hide resolved
@mbostock
Copy link
Member

This PR adds brushEvent.on so that you can register event listeners for the duration of a brush gesture, as d3-drag does. (I see you implemented that for d3-zoom, too.) What is the motivation for adding this functionality? I feel like it was needed for d3-drag because you can drag different things, but it’s overkill for d3-brush and d3-zoom because there’s only one “thing” (i.e., no per-gesture subject) that you’re interacting with. But I haven’t thought about this very much, so this is just an instinctual reaction.

@mbostock mbostock mentioned this pull request Jul 23, 2020
5 tasks
@Fil
Copy link
Member Author

Fil commented Jul 23, 2020

brushEvent.on

This was for consistency of the API and of the code base.

First, I'm not sure these derived events are ever necessary, even in d3-drag (or, we should have a more purposeful example than https://github.com/d3/d3-drag#event_on ). But it seems that it can give cleaner code, in particular if we want to create utilities like versor dragging that are a bit complex and might be applied on drag.start, or if we want to adapt the gesture to what is being manipulated.

For d3-brush I don't have a credible use case in mind. Still, I'm a bit confused by the “only one thing” argument. For a counter-example we have https://observablehq.com/@d3/brushable-scatterplot-matrix (which doesn't need brushEvent.on, but could use the same closure logic as in https://github.com/d3/d3-drag#event_on ).

For d3-zoom, I think its API should be as close as possible to d3-drag's: in fact we can use zoom as an (almost) drop-in replacement for drag, for example to manipulate a bunch of pictures (https://observablehq.com/d/8de812d302552f34).

@mbostock
Copy link
Member

I guess what I’m saying is that I don’t have a great justification for dragEvent.on, either, so I question whether it’s worth copying that to the other modules if it just makes the code more complicated.

@Fil
Copy link
Member Author

Fil commented Jul 23, 2020

Agree. I think the closure makes this fork of versor dragging a bit cleaner (removing the globals reusable variables r0, p0). Is it compelling enough? If not let's remove it everywhere.

@mbostock
Copy link
Member

Does that dragging-with-attitude notebook handle multitouch? I can’t really tell by looking at the code and I’m not on a touch device.

@Fil
Copy link
Member Author

Fil commented Jul 23, 2020

The multitouch version is https://observablehq.com/d/7a912fba32fbe641

the relevant part of the code is sending [x, y, angle] instead of [x, y]:

    return tl > 1
      ? [
          d3.mean(t, p => p[0]),
          d3.mean(t, p => p[1]),
          Math.atan2(t[1][1] - t[0][1], t[1][0] - t[0][0])
        ]
      : t[0];

and it's consumed by

  if (pt[2]) {
      a = attitude()
        .axis(projection.rotate([0, 0]).invert(pt))
        .angle(-(pt[2] - a0) * (180 / Math.PI))
        .compose(a);
    }

@mbostock
Copy link
Member

Do you think that having zoomEvent.on would make the multitouch easier? My guess is no but you wrote the code. :)

@Fil
Copy link
Member Author

Fil commented Jul 23, 2020

A multitouch fork using zoomEvent.on is here https://observablehq.com/d/0d284a21aa8e4aa4
I'd say the difficulty is exactly the same.

@mbostock
Copy link
Member

I vote for removing zoomEvent.on and brushEvent.on until we have evidence to believe they will be useful.

Fil and others added 2 commits July 24, 2020 14:22
Co-authored-by: Mike Bostock <mbostock@gmail.com>
Fil added a commit to d3/d3-zoom that referenced this pull request Jul 24, 2020
@Fil Fil mentioned this pull request Aug 18, 2020
@Fil Fil merged commit 89cdbf1 into master Aug 23, 2020
@Fil Fil deleted the two branch August 23, 2020 21:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants