chrvadala / react-svg-pan-zoom

:eyes: A React component that adds pan and zoom features to SVG
https://chrvadala.github.io/react-svg-pan-zoom/
MIT License
684 stars 126 forks source link

Add support to continue panning when outside the current viewport #160

Closed krnlde closed 3 years ago

krnlde commented 5 years ago

I moved the mouseMove and mouseUp (as well as their touch counterparts) upwards to the document. This way you're able to continue panning even when outside the current viewport. This bugged me the most on this library and I hope to find consent with this.

Cheers.

chrvadala commented 5 years ago

I was wondering if touching the global environment might have some drawback. We did it for onWheel event, but just because it was the only available option. In general I think that React works well when you use React events avoiding to directly touch browser event. Anyway I like this feature, What if we introduce a flag to selectively subscribe on global scope (your PR) rather than Component scope (AS IS)?

krnlde commented 5 years ago

Hey man! Thanks for you feedback :) Personally I have no concerns going to the global environment. In my codes I do it all the time and never had issues (as long as you don't want to support browser <IE11, because of attachEvent vs addEventListener). The flag you mentioned sounds great, but is the benefit worth the relatively complex logic for either putting all the events in the document vs. the viewport. Sounds like a lot of code duplication to me. Suggestions?

chrvadala commented 5 years ago

Hi @krnlde, thanks for your answer. I agree with you when you say that touching the global environment in personal project doesn't make issues, but here is a bit different. This is a library used by a lot of developers and we don't know what browser or spaghetti code are they managing 😄 React gives us a good abstraction from those things. In general I prefer to rely on React as much as possible, but I understand that sometimes we need to break this general rule. For the onWheel we did it, but it was required. For other events I prefer to allow the developer to select when use standard React events or global events. For concerns on code duplication le me say that @TimVanMourik and I are working to provide a new React version based on hooks, so don't worry if you need to copy paste some code, it is going to be refactored as soon as the hooks version is ready. Thought?

coveralls commented 5 years ago

Coverage Status

Coverage decreased (-0.7%) to 21.416% when pulling 200423400d50485b82cfab5cd9bccd1aab1791ca on krnlde:master into 9a9930be28533df867bbc0f2e4759d881972ad8a on chrvadala:master.