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

fix: Android onLoad event when view width and height are zero #1

Closed

Conversation

thomas-coldwell
Copy link

Copied from the original PR to react-native-fast-image here DylanVann#953

This PR aims to fix an existing issue with the onLoad event not firing on Android when the view style's width or height is zero as documented here DylanVann#865. There is a similar PR DylanVann#446 that was opened, but as mentioned in reviews this causes crashes due to the .override(Target.SIZE_ORIGINAL) method being applied to Glide. Instead, this PR follows the recommended way of getting the natural image dimensions as documented here in the Glide repo bumptech/glide#781 (comment).

The changes here include:

  • Adds a second target of a Size class with the relevant decoder + transcoder to achieve this
  • Aligns the onLoad method on Android to work in the same way iOS does - reporting the original natural image dimensions
  • Some minor dev setup fixes to get the example app up and running

These changes are best tested with the AutoSize component in the example app - as you can see setting the width to zero still triggers the onLoad event and this example is then rendered correctly.

@Beamanator
Copy link

@thomas-coldwell I believe we can close this since we took care of these changes in Expensify/App#13304 (using patch-package) right?

@Beamanator Beamanator closed this Jan 18, 2023
@cristipaval cristipaval reopened this Mar 20, 2023
@cristipaval
Copy link

cristipaval commented Mar 20, 2023

Hey @Beamanator ! I reopened this one because we have one more fix and we prefer forking the repo for now, instead of patching multiple PRs.

@cristipaval cristipaval self-assigned this Mar 20, 2023
@cristipaval cristipaval self-requested a review March 20, 2023 10:43
@cristipaval cristipaval removed their assignment Mar 20, 2023
@ArekChr ArekChr mentioned this pull request Mar 22, 2023
@cristipaval
Copy link

cristipaval commented Mar 22, 2023

Closing this one again in favor of another PR which migrates all patches to our fork.

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.

3 participants