Page MenuHomePhabricator

[native] replace out of box BottomSheetModal with basic BottomSheet
ClosedPublic

Authored by ginsu on Oct 5 2023, 10:58 AM.
Tags
None
Referenced Files
F3346536: D9373.diff
Fri, Nov 22, 9:04 AM
Unknown Object (File)
Tue, Nov 19, 5:57 PM
Unknown Object (File)
Sat, Oct 26, 1:15 AM
Unknown Object (File)
Sat, Oct 26, 1:15 AM
Unknown Object (File)
Sat, Oct 26, 1:15 AM
Unknown Object (File)
Sat, Oct 26, 1:15 AM
Unknown Object (File)
Sat, Oct 26, 1:15 AM
Unknown Object (File)
Sat, Oct 26, 1:12 AM
Subscribers

Details

Summary

The next part of viewing a full screen avatar for user profiles will be to introduce a UserProfileAvatarModal component using our FullScreenViewModal that we just introduced.

Outlined below are the steps I will take in this stack (each point here will be it's own diff):

  1. Replace out of box BottomSheetModal with basic BottomSheet (Unfortunately, BottomSheetModal does not play nice with nested navigator)
  2. Polish up the BottomSheetBackdrop animation
  3. Introduce User Profile Bottom Sheet Navigators (this is so we can navigate to User Profile Avatar Modal from the User Profile Bottom Sheet)
  4. Introduce the User Profile Avatar Modal component and all the necessary things (like route names) that we will need to navigate to this new screen
  5. Factor out the User Avatar component in UserProfile into it's own separate component to keep things better organized

When I first implemented our BottomSheet component, I opted to use the out of box BottomSheetModal since it had a great modal presentation view, and a backdrop that was really well animated. However, when I tried nesting a navigator inside the BottomSheetModal (in order to navigate to the User Profile Avatar Modal), I was running into some navigation errors that were crashing the app. After some more digging I found out that BottomSheetModal is just a bottom sheet wrapped in @gorhom/portal.

https://github.com/gorhom/react-native-bottom-sheet/discussions/668#discussioncomment-4946786

What this means is that the bottom sheet component is rendered on top of everything, which is fine for simple components/bottom sheets, but if we need to do any navigation this won't work.

Luckily migrating our BottomSheet component to use the standard/basic bottom sheet instead of the BottomSheetModal does not require that many changes

Test Plan

Confirmed that the bottomsheet worked (there is a small regression with the backdrop animation, but a subsequent diff in this stack will address this)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

ginsu requested review of this revision.Oct 5 2023, 11:20 AM
ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: atul, inka.
ginsu added inline comments.
native/user-profile/user-profile-bottom-sheet.react.js
43 ↗(On Diff #31688)

This method is exclusive to BottomSheetModal

https://gorhom.github.io/react-native-bottom-sheet/modal/methods

Looks good, let's make sure to fix import before landing

native/bottom-sheet/bottom-sheet.react.js
3 ↗(On Diff #31688)

Can we do

import GorhomBottomSheet from '@gorhom/bottom-sheet';

instead

This revision is now accepted and ready to land.Oct 5 2023, 4:17 PM

rebase one more time before landing (had a phab error when attempted to recently land)

This revision was landed with ongoing or failed builds.Oct 5 2023, 6:38 PM
This revision was automatically updated to reflect the committed changes.
native/user-profile/user-profile-bottom-sheet.react.js
36

You're not using this bottomSheetRef anymore, so it should've been deleted

native/user-profile/user-profile-bottom-sheet.react.js
36