Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

fix: Allow zooming on full child content, even if it appeared truncat… #70

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AmauryLiet
Copy link
Contributor

…ed on first render

Hey, I'll try to explain the issue here

Exemple use case:

  • ReactNativeZoomableView with an aspect ratio of 0.5 (twice taller than wide)
    XX
    XX
    XX
    XX
  • Child with an aspect ratio of 1 (width=height)
    YYYY
    YYYY
    YYYY
    YYYY

Then on first render the child won't be fully displayed (1st & 4th column hidden). This is fine.

However pan gesture does NOT allow to see the 1st & 4th column, whatever the gesture.
(Given minZoom={1} and bindToBorders!==false)

Ideally, I would include a minimal reproduction example & show the prop usage in the demo app, but I'm afraid to be lacking time is the near future :/

Copy link
Contributor

@SimonErich SimonErich left a comment

Choose a reason for hiding this comment

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

@AmauryLiet Thank you for your MR.
Awesome feature, that I would really love to add.

Could you please just add the two small things I mentioned in the review and then we are good to go. 👍

// W = H * RATIO

const aspectRatioOnMount = originalHeight && originalWidth ? originalWidth / originalHeight : 1;
const { contentAspectRatio = aspectRatioOnMount } = this.props;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please put this to the top of the method.
I would like to follow the best practice to have all external (prop) definitions at the top of the methods to know what we really use within our methods immediately.
(we need to convert some of the other occurences this.props... to, but let's start here.

const aspectRatioOnMount = originalHeight && originalWidth ? originalWidth / originalHeight : 1;
const { contentAspectRatio = aspectRatioOnMount } = this.props;

const wasWidthCroppedOnMount = aspectRatioOnMount < contentAspectRatio;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you please add a small comment explaining the logic here? It seems obvious, but up until now we kind of follow the approach to summarize the following block of logic, to make it easier to get into. :)

@AmauryLiet AmauryLiet force-pushed the fix/stop-overlimiting-gesture-when-binding-to-borders branch from f7082d9 to cb95ddd Compare June 18, 2021 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants