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

Chore update bottom sheet #927

Merged
merged 7 commits into from
Feb 5, 2025
Merged

Chore update bottom sheet #927

merged 7 commits into from
Feb 5, 2025

Conversation

ErikSin
Copy link
Contributor

@ErikSin ErikSin commented Jan 30, 2025

Implemented custom bottom sheet using React Native Reanimated, and React Navigation

Bottom sheets are now navigated to use React Navigation, and are of presentation: "transparentModal. Follows React Navigation practices for creating a modal.

Created a < BottomSheetWrapper />. This wrapper is used to create a transparent backdrop that doesnt not allow the user to interact with the screen in the background. It also enables the animation for the bottoms sheet to slide up on open and slide down on open.

The bottom sheet wrapper also prevents the user from going back when the android back button is closed.

This PR also replaces the bottom sheets in sync setting (to demonstrate how they are used)

closes #907

Screen.Recording.2025-01-30.at.2.49.15.PM.mov

To do

  • incrementally replace all instances of the Gorham bottom sheet with this new bottom sheet architecture

@ErikSin ErikSin requested review from cimigree and achou11 January 30, 2025 20:08
Copy link
Contributor

@cimigree cimigree left a comment

Choose a reason for hiding this comment

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

Seems to work well although for some reason the animation seems a tad more abrupt than it used to... but nothing blocking.
I tested this on various emulators and my two devices and the back button behavior was as expected and outlined.
I understand how this fits with the new architecture guidelines and that code separation is nice and clear.
I imagine that once we implement other modals, we will see if there are things to tweak.
Like maybe for some modals we might want to have a dismiss on backdrop tap enabled.
And I am a little nervous about this from the guidelines: "Modal screens should be the last in the stack - avoid pushing regular screens on top of modals." with audio.
I imagine you are already thinking about this, but perhaps there should be a plan in place about when and which to implement for other bottom sheet uses.

@ErikSin ErikSin mentioned this pull request Feb 3, 2025
@ErikSin
Copy link
Contributor Author

ErikSin commented Feb 4, 2025

the animation seems a tad more abrupt than it used to... but nothing blocking.

I can change the animation if you think that desirable. Was it just the speed? Or was there any other aspect

And I am a little nervous about this from the guidelines: "Modal screens should be the last in the stack - avoid pushing regular screens on top of modals." with audio.

We can push modal on top of other modals. Is there a time when there is a normal screen above the modal in the audio flow?

perhaps there should be a plan in place about when and which to implement for other bottom sheet uses.

Ideally we just get rid of the gorhom bottom sheet completely...

@cimigree
Copy link
Contributor

cimigree commented Feb 4, 2025

the animation seems a tad more abrupt than it used to... but nothing blocking.

I can change the animation if you think that desirable. Was it just the speed? Or was there any other aspect

Yes I think the speed.

And I am a little nervous about this from the guidelines: "Modal screens should be the last in the stack - avoid pushing regular screens on top of modals." with audio.

We can push modal on top of other modals. Is there a time when there is a normal screen above the modal in the audio flow?

Not sure if this is fitting the description (what is "above", exactly?), but when we go from granting permission to use the audio to the CreateRecording screen?

perhaps there should be a plan in place about when and which to implement for other bottom sheet uses.

Ideally we just get rid of the gorhom bottom sheet completely...

I meant for replacing the existing bottom sheets. Which do we do when and in what order like

@ErikSin
Copy link
Contributor Author

ErikSin commented Feb 4, 2025

Not sure if this is fitting the description (what is "above", exactly?), but when we go from granting permission to use the audio to the CreateRecording screen?

In this case we should use navigation.replace(...screen). This will replace the bottom sheet with the new screen while keeping the animations nice

@ErikSin ErikSin merged commit b4c9574 into develop Feb 5, 2025
5 checks passed
@ErikSin ErikSin deleted the chore-update-bottom-sheet branch February 5, 2025 15:59
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.

Implement New Bottom Sheet
2 participants