-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Remove SCREEN from lightbox layout #6124
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
When exploring Android animation, I noticed its content jumps on the first frame. I think this should help prevent that.
gaearon
force-pushed
the
lightbox-again-1
branch
from
November 5, 2024 22:38
8b62f84
to
e7db61e
Compare
arcalinea
temporarily deployed
to
lightbox-again-1 - social-app PR #6124
November 5, 2024 22:38 — with
Render
Destroyed
mozzius
approved these changes
Nov 6, 2024
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.
Bug confirmed fixed, could not see any regressions
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I want to get rid of these constants. They're subtly wrong in some cases, and it's very hard to think about the discrepancies due to safe insets. I want to change the code so that we rely on
SafeAreaView
and then everything is derived from its size.I'm not entirely sure this approach is the way to go, but this constant just sucks so I think it's a step forward.
The approach is to use alternatives to hardcoding
SCREEN
:measure(safeAreaRef)
. This always gives the height matching the area where we draw the image.<SafeAreaView>
where possible.<SafeAreaView>
code is native.useSafeAreaFrame()
.useSafeAreaInsets
if needed and subtract whichever ones we need manually.Test Plan
This fixes incorrect positioning on Android device when double-tapping into an image. Verify that double-tapping on main shows a black bar beneath, but double-tapping in this branch does not. (This doesn't repro on emulator for me.)
Verify controls (header, footer) work like before. Verify buttons are clickable. With no alt text, short alt text, and very long alt text. Verify very long alt text expands on tap but the scroll view doesn't overlap the header controls. Verify that controls still work and display correctly when invoking them while zoomed in.
Verify you can pan and pinch into the image.
Pinching into the image on device produces occasional flicker on Android, but this problem exists on main too. I'll need to look into that separately.(Fixed in #6126)It would be good to check what happens with super tall images.