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
681 stars 127 forks source link

SVG viewbox support (2.0) #150

Closed TimVanMourik closed 4 years ago

TimVanMourik commented 5 years ago

This pull request is analogous to #88 by @kheyse-oqton but updated and merged with the latest version of this library. It facilitates the use of the viewBox property of the main svg.

FROM:

<svg 
  width={1440} 
  height={1440}
>

TO:

<svg
  width={1440}
  height={1440}
  viewBox={`400, 100, 1440, 1440`}
>

This centers the view of the main svg and the miniature around the specified viewBox.

The only obvious side effect for current users is that in the unlikely scenario that they have viewBox specified, the centering will be affected. This is an unlikely case, because would be an unused property in the current version of this component.

chrvadala commented 5 years ago

Thanks for your PR, I'll study it. It would be better to have a solution that doesn't break current users. What if we introduce a new prop called considerVieport? This give to users a way to selectevely chose if handle viewport or not. Moreover have you tested mouse events? Are click coordinates handled in the right way?

TimVanMourik commented 5 years ago

Sure, I'll rename the prop to withViewbox!

Could you tell me what to test specifically? Whether the mouse panning behaviour is still correct? Or more than that?

TimVanMourik commented 5 years ago

I updated the PR to use the withViewBox prop. If the viewbox is specified, the width and height are not used anymore because the viewbox also specifies width and height. Currently, there is no error or warning if that's the case. That could be included in viewer.js:619, if desired.

Current behaviour is:

<ReactSVGPanZoom>
  <svg
  // width={1440}
  // height={800}
    withViewBox={`200, 100, 1400, 800`}
  >
      ...
  <svg>
</ReactSVGPanZoom>

I believe all events are updated correctly, fit to selection, zoom (on view center), and pan.

TimVanMourik commented 5 years ago

You can check out (a test deployment of) the application for which I'm using this component here: https://giraffe-tools-test.herokuapp.com/workflow/TimVanMourik/SomeGiraffeExample

This is already linking the update from this PR.

TimVanMourik commented 5 years ago

ping

chrvadala commented 5 years ago

Hi @TimVanMourik , thanks for your contribution and sorry for the delay. Here there're some change requests:

When written literally, a number is either an integer, or zero or more decimal digits followed by a dot (.) followed by one or more decimal digits and optionally an exponent composed of "e" or "E" and an integer. It corresponds to the production in the CSS Syntax Module [CSS3SYN]. As with integers, the first character of a number may be immediately preceded by - or + to indicate the number’s sign. https://www.w3.org/TR/css3-values/#numbers

TimVanMourik commented 5 years ago

Thanks for the review, @chrvadala! I fixed each of the four points in the respective commits.

TimVanMourik commented 4 years ago

pong

chrvadala commented 4 years ago

Sorry, this weekend I'm out... I should be able to work on it next week.. apologize for the delay

chrvadala commented 4 years ago

Released with version 3.3.0 @TimVanMourik and @kheyse-oqton Thanks for your work, I added you in contributors list

TimVanMourik commented 4 years ago

Nice, thanks! :D