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

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