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

Interactivity refactor #112

Merged
merged 123 commits into from
Oct 14, 2019
Merged

Interactivity refactor #112

merged 123 commits into from
Oct 14, 2019

Conversation

luucvanderzee
Copy link
Collaborator

@luucvanderzee luucvanderzee commented Oct 11, 2019

  • Refactored interaction code, made handlers a lot easier to read (fixes [Interactivity] Refactor InteractionHandlers and make it easy to make custom ones #98)
  • Changed names of interactions (fixes [Interactivity] Abstraction level of interactions #106)
  • Implemented select (fixes [Interactivity] Selection #107)
  • Added blockReindexing option for Graphic, Section, Marks and Layers (this is useful when drawing new geometries that have interactions attached to them, like drag. If you are dragging, you don't want the mark/layer to reindex every time the data changes)
  • Changed createZoomHandler and createPanHandler structure (might be a bit awkward, I'm open for improvements to this)
  • Fixed bug of double re-rendering when section's scales and mark/layer's data change at the same time
  • Ported over better geometryUtils from transshape
  • Added examples for the demonstration I did for the meetup talk (are now in a separate folder, not imported anywhere)

TODO:

  • Mobile

export let onDragend = undefined
export let onMousedrag = undefined

// Touch interactions
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed the distinction between mouse and touch interactions here. If I'm not mistaken the original event manager provided only one 1 API that uses either mouse or touch in the background based on whatever detectIt detects to be present. Is this a change from that approach?

Copy link
Collaborator Author

@luucvanderzee luucvanderzee Oct 14, 2019

Choose a reason for hiding this comment

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

Yes, mouse and touch interactions are now completely separated. Touch does not have an equivalent to mouseover and mouseout, and mousemove works differently for touch too. To force everything under one API meant that we had to create a touch equivalent of mouseover and mouseout, which we did by looking at the timestamps of the events, calculating the time elapsed between down and up etc. There were two problems with this approach:

  1. The 'treshold' that determined whether something was a touchover or a touchdown was fixed and unchangeable, and you might want to have more control over this
  2. The 'treshold' is only necessary when you needed both touchover and touchdown. This is not always necessary and in fact should be avoided when possible IMO. If you just need to know whether the user put his/her finger down, and you don't care for how long, you now have a problem, because the 'treshold' is built in. We can still support different interactions by letting the user take care of this 'treshold', which will be a bit harder, but that's a good trade-off for a more flexible and robust API I think (and also to discourage this 'different-interactions-based-on-how-many-milliseconds-your-finger-is-down' behavior).

@@ -0,0 +1,3 @@
export default function (callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we use Svelte's built-in tick() instead? https://svelte.dev/docs#tick

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah I was looking for something like this but couldn't find it. I will use this instead

@atepoorthuis
Copy link
Contributor

The netlify deploy fails but we can ignore this since we're restructuring the site/ part anyways right now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants