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

SVG viewbox support (2.0) #150

Merged
merged 18 commits into from
Sep 17, 2019
Merged

SVG viewbox support (2.0) #150

merged 18 commits into from
Sep 17, 2019

Conversation

TimVanMourik
Copy link
Contributor

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
Copy link
Owner

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
Copy link
Contributor Author

TimVanMourik commented Aug 27, 2019

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

ping

@chrvadala
Copy link
Owner

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

The value of the ‘viewBox’ attribute is a list of four numbers , , and , separated by whitespace and/or a comma,
https://www.w3.org/TR/SVG/coords.html#ViewBoxAttribute

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

  • Studying this PR I realised that the side effect that you cited in your first comment is not so common. I'll release this PR as a new minor release, so in this case you can replace withViewBox again to viewBox. Sorry for that, it wasn't clear before.

Copy link
Owner

@chrvadala chrvadala left a comment

Choose a reason for hiding this comment

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

* in order to reuse the standard name as defined in SVG RFC https://www.w3.org/TR/SVG/coords.html#ViewBoxAttribute, please rename SVGViewBoxX and SVGViewBoxY with SVGMinX and SVGMinY
chrvadala#150 (comment)
* please move the ViewBox parser in a new file called utils/ViewBoxParser.js

chrvadala#150 (comment)
This should do the trick and is more readable than a regex, IMO

chrvadala#150 (comment)
@TimVanMourik
Copy link
Contributor Author

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

@TimVanMourik
Copy link
Contributor Author

pong

@chrvadala
Copy link
Owner

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

@chrvadala chrvadala merged commit c69b899 into chrvadala:master Sep 17, 2019
@chrvadala
Copy link
Owner

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

@chrvadala chrvadala mentioned this pull request Sep 17, 2019
@TimVanMourik
Copy link
Contributor Author

Nice, thanks! :D

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

Successfully merging this pull request may close these issues.

2 participants