Add prop zoomAlignmentY to control zoomed image vertical alignment relative to the viewport #883
Conversation
… alignment to viewport
…l how translateY get calculated
README.md
Outdated
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| [](https://www.npmjs.com/package/react-medium-image-zoom) [](https://bundlejs.com/?q=react-medium-image-zoom) [](https://www.npmjs.com/package/react-medium-image-zoom) [](#contributors-) | |||
|
|
|||
| ## Disclaimer | |||
| this package is a fork of [react-medium-image-zoom](https://github.com/rpearce/react-medium-image-zoom/) with added functionality waiting to be merged in upstream. | |||
| see pull requests for more details: https://github.com/rpearce/react-medium-image-zoom/pull/883 | |||
There was a problem hiding this comment.
I don't think this should be in the PR
There was a problem hiding this comment.
Sorry, this wasn't intentional. I needed to publish this ASAP while waiting but it wasn't intended for the pull request, it's been removed.
package.json
Outdated
| @@ -35,7 +38,8 @@ | |||
| "Yida Zhang <[email protected]>", | |||
| "eych", | |||
| "tshmieldev", | |||
| "David Edler (https://github.com/edlerd)" | |||
| "David Edler (https://github.com/edlerd)", | |||
| "Alessandro Stoppato <[email protected]>" | |||
There was a problem hiding this comment.
I don't think anything in this file should be in the PR
There was a problem hiding this comment.
Sorry, this wasn't intentional. I needed to publish this ASAP while waiting but it wasn't intended for the pull request, it's been removed.
source/Controlled.tsx
Outdated
| @@ -88,6 +89,7 @@ interface ControlledDefaultProps { | |||
| swipeToUnzoomThreshold: number | |||
| wrapElement: 'div' | 'span' | |||
| zoomMargin: number | |||
| zoomAlignmentY: 'center' | 'top' | |||
There was a problem hiding this comment.
Is there a reason for not including 'bottom'?
There was a problem hiding this comment.
Also, I would, alphabetically, put this before zoomMargin here and the other places in this file.
There was a problem hiding this comment.
I added the bottom value for zoomAlignmentY
there is an issue though, shared with the default center alignment: when zoomed image is wrapped inside a container having position: relative and a height which is not the same as viewport's, the position of the zoomed image is wrong.
It can be addressed by setting the translate property based on the first parent with position: relative instead of window.innerHeight but I don't see a way in the current implementation to get the zoomed image parent element in getStyleModalImg()
There was a problem hiding this comment.
I'll have a look when I can. I don't have a lot of time to work on this, but a little bit here and there.
There was a problem hiding this comment.
I understand. Anyway the issue is not related to the new functionality and should be fixed in another pull request.
I'll try to look into the stories when I have time to update that as well.
Hopefully it can be merged then.
There was a problem hiding this comment.
It's less a matter of the code changes and more a matter of: "Do I want to support this behavior forever?"
Other things I need to consider:
- Is there a different API to provide?
- What if somebody wants to move it to a predefined position on both the Y and X axes?
- Is any of this something that this library should do?
fix props order by making it alphabetical
Description
This pull request add a prop to and component
zoomAlignmentYthis props allow to control the zoomed image vertical alignment relative to the viewport; it defaults to
centerto keep the standard behavior consistent, but add also the optiontopto align the image to the viewport topThis is especially useful when used together with prop
ZoomContentand showing some additional content along the image when zoomed, for instance an extended image description: aligning the zoomed image to top leave a lot more viewport space availableTesting
<Controlled zoomAlignmentY='top'>[...]</ Controlled>zoomAlignmentYand it should align to center as asual