svgdotjs / svg.panzoom.js

A plugin for svg.js which enables panzoom for viewbox elements
MIT License
95 stars 54 forks source link

Adding level to zoom event #6

Closed dotnetCarpenter closed 7 years ago

dotnetCarpenter commented 7 years ago

I need to set a max/min zoom for my map, to prevent that the user zoom so far out that the map disappears.

In order to know when to use ev.preventDefault() I need to know if we're zooming in or out. Since panzoom handles the wheelZoom, the only way is to listen to the zoom event.

My current code rely on getting the level variable from the zoom function.

world.on("zoom", function maxMin(ev) {
  var newZoomLevel = world.zoom()
  var lvl = ev.detail.level
  var zoomingIn = newZoomLevel < lvl

  if(zoomingIn && newZoomLevel > 7) ev.preventDefault()
  else if(!zoomingIn && newZoomLevel < originalZoomLevel) ev.preventDefault()
})

If I just set my logic after newZoomLevel then the user is trapped at the zoom level where I call ev.preventDefault(), since wheelZoom skips some steps when getting to the desired zoom level (which is expected and sane). Imagine that newZoomLevel is 7.1833495 and I call ev.preventDefault(). Now, if the user tries to zoom out and wheelZoom goes to 7.021554, I will still call ev.preventDefault() since it's higher than 7, hence trapping the user in a non zoomable state.

Adding this.fire('zoom', {box: box, focus: point, level: level}) to the zoom function creates a misalignment with pinchZoom, since level doesn't exist (on less we substitute level with currentDelta).

So the question is:

  1. should I calculate level from box and not add level to zoom (not sure how to do that yet)
  2. add level to zoom but not to pinchZoom
  3. calculate level in pinchZoom so the event is aligned
  4. use currentDelta as level in pinchZoom so the event is aligned

@Fuzzyma What do you think?

Fuzzyma commented 7 years ago

Well you could just save the lastZoomLvl and compare to that. This way you now where you are. You also could set the zoom lvl to 7 fixed when the new zoom is higher than that. Beside all this I believe that adding the level to the event details is a good thing. It is information which is often required and only in this event. So it is fine to only have it there

dotnetCarpenter commented 7 years ago

Do you mean save lastZoomLvl in my code that uses svg.panzoom.js? The problem is that I don't know the last zoom level unless I set it and when it comes to wheelZoom, it's svg.panzoom.js that handles all of that.

I will add level to the zoom event but I hate that the API is splitting. It makes it hard to write generalized code when arguments change like that. But for now, I'll let it rest. :)

dotnetCarpenter commented 7 years ago

On second thought. It's not a good idea to have different arguments to the same event. If I do this, then code that listen to zoom and works perfect with mouse input will fail when used with touch input.

dotnetCarpenter commented 7 years ago

Perhaps the zoom event in pinchZoom should be removed?

Fuzzyma commented 7 years ago

I mean saving the zoom level on every zoom. You intialize lastZoom with your initial zoom and ... uh - i got your point. What about doing this on zoomStop?

We also can divide the events into zoom and pinchZoom

dotnetCarpenter commented 7 years ago

zoomStop was removed to simplify things. We currently have the following events:

Event Name Argument Value preventDefault support
zoom { box, focus, level } YES
panStart { event } NO
panEnd { event } NO
pinchZoomStart { event } YES
pinchZoomEnd { event } NO
dotnetCarpenter commented 7 years ago

Currently there is a bug in the doc or implementation - you can not use preventDefault() on the zoom event if it comes from pinchZoom.

I've fixed it in the level branch but not sure if the implementation should stay like that.

dotnetCarpenter commented 7 years ago

We also can divide the events into zoom and pinchZoom

Maybe - I'll need to think it over a bit

Fuzzyma commented 7 years ago

Mh I think theer was a reason why preventing is not possible there. Imo zoom of pinchzoom should be preserved in the pinchzoomstart and not in the zoom event. But Iam not sure about that one

dotnetCarpenter commented 7 years ago

I don't have a touch device with me. I'll need one before I can say anything clever :)

inanisvitae commented 7 years ago

Why don't you just put min and max as parameters in the zoom in the first place?

dotnetCarpenter commented 7 years ago

@kievking You mean adding max/min as an option to panZoom()? Zoom via mousewheel and pinchzoom is handled by svg.panzoom.js and is not something you call yourself, so max/min can not, currently, be set from outside.

Fuzzyma commented 7 years ago

@dotnetCarpenter https://github.com/svgdotjs/svg.panzoom.js/pull/10

dotnetCarpenter commented 7 years ago

Closed in favor of #10