-
Notifications
You must be signed in to change notification settings - Fork 14
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
Replace GIFs with new GIF viewer #1437
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is so clean, I love it. I did find some weirdness when rotating gifs. See the two below cases:
- I expect this gif to fill the screen vertically when in landscape, but there is a gap at the bottom:
note1gwug9ke6x40uk8qku0fy0eszr7jldfwj4ap6meqzqhcr3jvyg7kswym5zs
RPReplay_Final1724944971.MP4
- This gif has a weird gap in portrait after I pinch to zoom a bit in landscape:
note19h0x9um6ymwnmmnjqnc6mvmfpcnqq0t0qmam009fwdlwnzlmrqgq76aptp
RPReplay_Final1724944904.MP4
@@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
- Added a feature flag toggle for “Enable new media display” to Staging builds. [skip-release-notes] | |||
- Added a new gallery view to display multiple links in a post. Currently behind the “Enable new media display” feature flag. [skip-release-notes] | |||
- Show single images and gallery view in the proper orientation. Currently behind the “Enable new media display” feature flag. [skip-release-notes] | |||
- Added an overlay to GIFs that plays the animation when tapped. Currently behind the “Enable new media display” feature flag. [skip-release-notes] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oooh, what is is [skip-release-notes]
. Does this prevent them from showing up on TestFlight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the idea. Right now it's 100% manual; maybe in the future it'll be 100% automated. And maybe #1438 will change the approach slightly.
.placeholder { | ||
WebImage( | ||
url: imageUrl, | ||
content: { image in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an API change on SDWebImage? It stinks that we have to duplicate a bunch of the view modifiers now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unfortunately the old API is gone. I can look and see if we can do anything better, or maybe try to unduplicate the view modifiers.
Unfortunately I think these are general issues with our ImageViewer; the same weird things happen for me when rotating plain old images: I'll look and see what I can do. |
In that case I approve and we can address that in another PR. |
Yeah, I ended up debugging a bit and documenting my findings in a Notion doc. I'll create a ticket, add my notes to it, and we can address it when it's prioritized. |
New ticket to address issues: #1440 |
Issues covered
#1168
Description
Like it says in the title. GIFs now appear in the feed, profile, and thread with a "GIF" button on top of them.
How to test
Screenshots/Video
Simulator.Screen.Recording.-.iPhone.SE.3rd.generation.-.2024-08-28.at.15.38.56.mp4