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

Added option to align svg while fitting it to the viewer (#65) #120

Merged
merged 5 commits into from Jul 29, 2018
Merged

Added option to align svg while fitting it to the viewer (#65) #120

merged 5 commits into from Jul 29, 2018

Conversation

ghost
Copy link

@ghost ghost commented Jul 17, 2018

Alignment can be passed as optional props SVGAlignX and SVGAlignY to ReactSVGPanZoom and Toolbar components; or via arguments to the base fitToViewer function.
(What is passed to ReactSVGPanZoom will also be passed to the toolbar.)

Default alignment values are provided - aligning to top left, as before.

I will need the ability to fit and center svgs in one of my projects, so I thought I might as well write a bit more general solution for left/center/top horizontal alignment and top/center/bottom vertical alignment..

Alignment can be passed as optional props to ReactSVGPanZoom and Toolbar components or as arguments to the base fitToViewer function.

Default alignment values are provided (aligning to top left, as before).
@ghost
Copy link
Author

ghost commented Jul 17, 2018

The prop names are probably misleading, now that I think of it.
For ReactSVGPanZoom they may suggest that svg will be aligned when first rendered - not only when fitToViewer is called explicitly.
Perhaps that would actually be desirable to have an option to align svgs, even without fitting them?

@chrvadala
Copy link
Owner

Thanks for your PR. It's a really useful feature and will close this issue
#65

I agree that is better to renames these two new props, because after the change of the method fitToViewer() to fitToViewer(SVGAlignX, SVGAlignY) you will have that only the toolbar will consume these values.

I think that we cold introduce a more general property that is toolbarProps
Thought?

@ghost
Copy link
Author

ghost commented Jul 18, 2018

Oh sure, it's better to have fitToViewer(SVGAlignX, SVGAlignY) than just fitToViewer() - and so toolbarProps would totally make sense.
I could change both things, but I'm not sure what other props you would like to have in toolbarProps.

BTW, thanks for the awesome project! - really useful! 😃

@chrvadala
Copy link
Owner

You are right, probably there're other props that they would be better moved into toolbarProps, but move them breaks the current API and I think that is something that we have to avoid. For now we can just move these two new props (SVGAlignY, SVGAlignX) in toolbarProps and keep the other things unchanged. In the future I will release a new major release and I'll move also the remaining part. LMK
Don't forget to add your name in README.md ;-)

Plus fitToViewer() changed to fitToViewer(SVGAlignX, SVGAlignY) in ReactSVGPanZoom
@ghost
Copy link
Author

ghost commented Jul 21, 2018

Right, obviously it's better to defer moving other props till the next major release.
Only SVG alignment in the toolbarProps then!

@@ -531,6 +532,12 @@ ReactSVGPanZoom.propTypes = {
//Turn off zoom on double click
disableDoubleClickZoomWithToolAuto: PropTypes.bool,

//toolbar props
toolbarProps: PropTypes.shape({
Copy link
Author

Choose a reason for hiding this comment

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

I didn't restrict the shape of toolbarProps here in order not to restrict props for custom toolbars.
But it's surely safer this way :)

@chrvadala chrvadala merged commit e78e373 into chrvadala:master Jul 29, 2018
@chrvadala
Copy link
Owner

Thanks for your PR. I'm going to release with 2.18 version

@chrvadala
Copy link
Owner

Feature released with 2.18. Thanks ;)

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.

None yet

2 participants