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

[Lightbox] Always rely on Expo Image cache #6189

Merged
merged 7 commits into from
Nov 9, 2024
Merged

[Lightbox] Always rely on Expo Image cache #6189

merged 7 commits into from
Nov 9, 2024

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Nov 9, 2024

Removes our own caching layer for image dimensions. Instead, rely on onLoad callback from expo-image which tells us the dimensions. So if the image is cached by expo-image, these will come through via onLoad.

In the lightbox, until onLoad fires for the full image, we'll use either the embed record dimensions (if available), or fall back to the already fetched thumbnail dimensions (determined by onLoad in the feed).

What this gives us:

  • No more duplicate requests (via RN Image vs Expo Image) for missing dimension embed records
  • The lightbox is properly animated if onLoad in feed ran in time — since we know the aspect ratio early in that case

Test Plan

Verify that images animate when opening. Notably, images on posts like https://bsky.app/profile/docdre.bsky.social/post/3l6lzzqhxwh2o (which has no image dimensions in embed record) will also animate now.

Comment out the onLoad calls in AutoSizedImage.tsx and Gallery.tsx. Verify they aren't essential — the lightbox works without them. For posts with no dimensions in the embed (https://bsky.app/profile/docdre.bsky.social/post/3l6lzzqhxwh2o), the lightbox would not animate without those onLoad calls — similar as before this PR. It should still display though. Posts with dimensions in embed records should continue animating even without those onLoad calls.

Verify images appear cropped in feed similarly as before. Verify the animation from the cropped ones works.

Not sure how to verify this, but I'd expect that we no longer fire duplicate requests for images with missing dimensions.

Copy link

github-actions bot commented Nov 9, 2024

Old size New size Diff
8.01 MB 8.01 MB 4 B (0.00%)

Copy link
Contributor

@haileyok haileyok left a comment

Choose a reason for hiding this comment

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

makes sense, and works as described on the test image linked.

@gaearon gaearon changed the base branch from new-lightbox-open to main November 9, 2024 22:36
@arcalinea arcalinea temporarily deployed to save-image-dims - social-app PR #6189 November 9, 2024 22:36 — with Render Destroyed
@gaearon gaearon merged commit 42abd97 into main Nov 9, 2024
6 checks passed
@gaearon gaearon deleted the save-image-dims branch November 9, 2024 23:07
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