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.

Sign up for GitHub

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: image gallery issues #2835

Merged
merged 2 commits into from
Dec 10, 2024
Merged

fix: image gallery issues #2835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a88af85
fix: infinite image loading issue
isekovanic Dec 9, 2024
ec278ff
fix: image gallery animations issue
isekovanic Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package/src/components/Attachment/Gallery.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ const GalleryImageThumbnail = <
setLoadingImage(false);
setLoadingImageError(true);
}}
onLoadEnd={() => setLoadingImage(false)}
onLoadEnd={() => setTimeout(() => setLoadingImage(false), 0)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It appears that with at least Expo 52 (and new architecture enabled), the native Image component's onLoadStart and onLoadEnd began behaving weirdly.

Namely, Expo's caching strategies for images try to enforce an image with a specific url being returned from the cache whenever possible. This however (maybe in conjunction with some of our code), causes onLoadEnd and onLoadStart to happen seemingly at the same time, producing something along the following lines as logs:

(NOBRIDGE) LOG  LOAD END 1733752101420 https://us-east.stream-io-cdn.com/62344/images/0d0c0d5e-b113-429b-aab6-6cc1520f3c87.IMG_0005.JPG
 (NOBRIDGE) LOG  LOAD START 1733752101420 https://us-east.stream-io-cdn.com/62344/images/0d0c0d5e-b113-429b-aab6-6cc1520f3c87.IMG_0005.JPG

This in turn causes onLoadStart to be called after onLoadEnd and thus the loading indicator never disappearing. I realize the fix is quite hacky, but this way we guarantee the order by forcing onLoadEnd to go in the JS task queue.

A proper solution would be something along the lines of having a Set() of trace IDs (each trace being created during a render) and invoking onStartReached only if the current trace ID doesn't exist in the Set (i.e bypassing the situation where onEndReached might've been called inbetween). However, this is currently not needed and we'll keep it in mind in case it does become a problem.

onLoadStart={() => setLoadingImage(true)}
resizeMode={thumbnail.resizeMode}
style={[
Expand Down
4 changes: 2 additions & 2 deletions package/src/components/ImageGallery/hooks/useAnimatedGalleryStyle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,12 +72,12 @@ export const useAnimatedGalleryStyle = ({
{ scaleX: -1 },
{ translateY: yScaleOffset },
{
translateX: -translateX.value - xScaleOffset,
translateX: -xScaleOffset,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Translating by translateX.value as well causes horizontal scrolling to happen even if you're closing the gallery, meaning it'll happen regardless of whether IsSwiping.TRUE is set or not. We don't need this behaviour as we want swiping to be active only when actually swiping, which is instead already handled downstream within the DOM tree. We need the initial translation to make sure that the images stay in their correct position on first load.

},
{ scale: oneEighth },
],
};
}, []);
}, [index]);

return [animatedGalleryStyle, animatedStyles];
};
Loading