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

[WIP] Rewrite Android lightbox #1563

Closed
wants to merge 16 commits into from
Closed

[WIP] Rewrite Android lightbox #1563

wants to merge 16 commits into from

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Sep 28, 2023

Fixes #1485.

This is a comprehensive rewrite of the Android lightbox aligning it closer with behavior you see in stock Android apps. We're using Reanimated + RNGH since these provide more control and will let us compose with scroll views better.

Not ready for a full review yet.

  • Pinch and pan gestures
    • Basic interaction
    • Panning and pinching doesn't go offscreen (new!)
    • Pinching is centered around focal point (new!)
    • Panning is disabled while black bars are visible (new!)
    • Enforce max zoom level
  • Feature parity
    • Swipe away gesture
      • Basic implementation
      • Make it more tolerant X-wise
    • Double tap gesture
    • Show/hide controls based on zoom level
    • Loading indicator
      • Check why sometimes it appears when you don't expect (multiple images?)
  • The Long Tail
    • Regression: Pan during first pinch doesn't work
    • Existing bug: It's possible to spam gestures during scroll swipe
      • Fix overflowing images (e.g. quick double tap and swipe)
      • Fix the scroll view getting stuck (e.g. pinch gesture during swipe)
    • Existing bug: swiping may sometimes get stuck
  • Integration
    • Re-enable the "gallery" scroll view above (temporarily disabled)
      • Fix conflict with pinching
    • Verify we haven't regressed iOS
    • Delete dead code from old implementation
    • Inline weird helpers from old implementation
    • Fix types / lint
    • Move utility functions into own file

return [1, 0, 0, 0, 1, 0, 0, 0, 1];
}

function prependTranslate(t: Matrix3, x: number, y: number) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Wrote these as in-place mutations to avoid allocating a bunch of stuff during gestures

@gaearon gaearon force-pushed the rewrite-android-lightbox branch 3 times, most recently from c9b2674 to 115d60d Compare October 3, 2023 03:48
@pfrazee
Copy link
Collaborator

pfrazee commented Oct 3, 2023

Excellent work here, Dan. It's feeling good and consistent across devices and should set us up much better in the future!

A couple of notes:

  • The "swipe up to dismiss" animation feels a little too quick to me on iOS. Interestingly it felt exactly right on android. The gesture itself is perfect, but the slide & fade animation that's after gesture seems to take too much velocity from the gesture and therefore runs too quickly. EDIT: I just realized you only updated android, so ignore this!
  • A long-standing complaint is that the bottom bar can cover images that occupy the full phone screen. When we zoom the bottom bar goes away; is there any way we could add a tap-toggle on the image which does the same thing?
  • Android has a pretty significant delay between tapping an image and the lightbox showing. This might be due to a tap-recognition delay we've introduce to avoid accidental triggers (I vaguely recall something android-specific for that) but it's worth taking a look to see if there's something we can do there.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 3, 2023

Still unhappy with how pan and pinch gestures compose but maybe it's not blocking. Left a comment in software-mansion/react-native-gesture-handler#2616 (comment) for the RNGH maintainers.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 3, 2023

As for other notes, I'll do them in a follow-up. I want this PR to just get us to the baseline with the custom implementation. I'll polish up the code and make some videos to show the difference with what we had.

@gaearon gaearon force-pushed the rewrite-android-lightbox branch 2 times, most recently from 60665ec to 3c2654a Compare October 4, 2023 03:29
ta[8] = a20 * tb[2] + a21 * tb[5] + a22 * tb[8];
}

function readTransform(t): [number, number, number] {

Choose a reason for hiding this comment

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

t here will be any (and below in some following functions)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yea i haven’t started fixing up types yet

},
loading: {

Choose a reason for hiding this comment

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

This is provided by RN if that's useful https://reactnative.dev/docs/stylesheet#absolutefill

@gaearon gaearon force-pushed the rewrite-android-lightbox branch from e231105 to 26183ee Compare October 5, 2023 19:13
@gaearon gaearon closed this Oct 6, 2023
@gaearon gaearon deleted the rewrite-android-lightbox branch October 6, 2023 01:19
@gaearon
Copy link
Collaborator Author

gaearon commented Oct 6, 2023

I'll keep this for historical reasons but will resubmit cause a rebase was too hard and I needed to restructure things anyway.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 6, 2023

Resubmit: #1624

@gaearon gaearon mentioned this pull request Oct 6, 2023
24 tasks
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.

Pinch gesture gets stuck on Android
4 participants