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

Pass along mode to BrushEvent #75

Merged
merged 9 commits into from
Aug 26, 2020
Merged

Conversation

woutervh-
Copy link
Contributor

@woutervh- woutervh- commented Aug 18, 2020

Hello.

In my project I need to know the mode of the brush in the event handler.
This is because depending on which axis (x/y) the user is resizing the selection in, or whether the user is moving the selection without resizing (dragging), I need to change my logic on the snapping/clipping of the brush selection.

In this PR is my attempt at including the mode in BrushEvent.
I've tested it in my local project and it works fine.
Not sure if this is how you would solve it, so feel free to edit it.

@Fil
Copy link
Member

Fil commented Aug 18, 2020

We are almost ready to release d3-brush@2 with a modified API, so you will probably have to rebase it on #71 (or wait a few days for it to be merged).

Can you share a (real world) example of how this is useful?

@woutervh-
Copy link
Contributor Author

woutervh- commented Aug 18, 2020

Hi @Fil ,

Sure, here is our real world scenario:

We allow our users to draw 2D boxes in the plot, to indicate areas of interest.
We have a requirement that these boxes cannot overlap.
After the brush event happens, we want to .move the selection so that we can ensure the selection does not overlap with existing boxes.

For example: if the user is using the "north" handle to vertically expand the selection, and at some point reaches an existing box, then we ensure that the "north" handle does not move past the border of the existing box.

The way that the selection needs to be .move'ed depends heavily on the mode of the brush.
This is because it's OK to alter the "north" position of the selection when the user is attempting to move only the north handle, but when the user is dragging the entire selection, it would not be good to only alter the "north" position. Instead, the size of the box needs to be conserved and the center needs to be constrained so that the edges don't cause any overlap.

Hope this makes sense, and explains why we need information about the mode.

@woutervh-
Copy link
Contributor Author

I've created this notebook to illustrate the problem:
https://observablehq.com/@woutervh-/brush-with-collision

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

@mbostock mbostock left a comment

Choose a reason for hiding this comment

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

Looks good. Would benefit from updating the README to mention the new mode field.

@Fil Fil added the feature label Aug 23, 2020
@Fil
Copy link
Member

Fil commented Aug 23, 2020

v2 is now in master. It emits a specific brushEvent on each event.

That event is structured like so:

{
  type: "brush", // or "start", "end"…
  sourceEvent: MouseEvent {},
  target: ,
  selection: [[x0, y0], [x1, y1]]
}

I think that this is where we would now add mode: mode.name?

@woutervh-
Copy link
Contributor Author

Thanks @Fil

  • Added mode to README
  • Resolved merge conflicts from branch two

Copy link
Member

@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.

Works well 👍

src/event.js Outdated Show resolved Hide resolved
src/brush.js Outdated Show resolved Hide resolved
src/brush.js Outdated Show resolved Hide resolved
woutervh- and others added 3 commits August 25, 2020 11:29
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
Co-authored-by: Philippe Rivière <[email protected]>
@woutervh-
Copy link
Contributor Author

Good catch in the review, how did I miss this?? enenumerable: true
All suggestions applied.

@Fil Fil merged commit a7b4524 into d3:master Aug 26, 2020
@woutervh-
Copy link
Contributor Author

woutervh- commented Sep 14, 2020

FYI: here's my attempt at a solution to stop the brush from intersecting other rectangles, using the newly added functionality.

Some improvements can be made, but I will need more information from the brush's state to achieve it.
Maybe in a future issue/PR I will address these issues, but for now this is good enough for me:

https://observablehq.com/@woutervh-/brush-with-collision-solution-continued

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

3 participants