messageActionSheet : using Modal instead of @expo/react-native-action-sheet

[abhi0504]

@nikhilmaske-2001 reported an issue #4407 that is :
when we long press on any message then messageActionSheet opens up and it took a extra black screen on Share option when we share our message

here i and @gnprice have discussed about the problem and found that this issue coming from @expo/react-native-action-sheet for which i have made a tiny app and report this issue on facebook/expo page here and also created a snack for better explaination of the issue here

then we have decide to use Modal instead of @expo/react-native-action-sheetas it restricts us upto a certain extent on basis of UI for eg if we useModalinstead of@expo/react-native-action-sheet` then we have many options for eg colored messageActionSheet , popping out with diff animations , can set some icons before any option which and greg have
already discussed here

Also we can do many things if we use Modal and can tweak our messageActionSheet's UI to an another level for eg

• We can add icons in starting of different options of our messageActionSheet
• We can make our text colorful in messageActionSheet
• We can add different animations in our messageActionSheet

Fixes: #4407

[chrisbobbe]

Thanks for the PR, @abhi0504!

I see that GitHub is reporting some CI failures; let's get those fixed up. 🙂 If you're not sure how to fix something, please ask. You should start by running tools/test, and a list of problems will appear. I see failures reported by Flow, ESLint, and Prettier.

Also, could you please post some screenshots of the new UI on Android, in both night and day modes? Here are some for iOS:

---

• The first thing I notice is that this is very different from what it was on iOS before. 🙂 That might be OK; Apple's design guidelines do allow modal interactions. I think the decision to stop using Apple's native action sheet will depend mostly on how closely our modal's purpose matches what Apple says action sheets are for. I haven't yet thought deeply enough about this; in particular, we'd want to consider the future direction of the action sheet, including the follow-up improvements you've suggested. For reference, here's how the action sheet looks on iOS before the changes in this PR:

  

• I also notice that, at least on iOS (I haven't tested on Android yet) I can't tap somewhere above the top of the action sheet to make it disappear (this was possible before); I have to tap the cancel button. Is there an easy fix to let me tap above the top of the action sheet to make it disappear?

• Another thing I notice is that, if I've rotated the device into a landscape orientation, something weird happens: everything seems to turn sideways when I open the action sheet, as though it's trying to go back to a portrait orientation. Then, when I close the action sheet, things turn right-side-up again. Here's what it looks like:

  Feb-19-2021.17-14-12.mp4

  Do you see this happening on Android too? That could provide a clue for debugging.

[abhi0504]

Thank you so much @chrisbobbe for your reply
I am fixing CI failures and need some help in fixing them for now some of them are fixed and some of them are in process

Here are the screenshots of the new UI on Android, in both night and day modes

Also you have mentioned that there is a different UI in ios which might be OK for now also we can change that in future commits using Platform

you have mentioned that on clicking outside the messageActionSheet the modal must be closed so for that I have added an commit here If you would say then i will squash both commit into one
Here is a video which can reffer above statement

Android.Emulator.-.Pixel_3_API_29_5554.2021-02-20.14-54-33.mp4

Another thing that you have noticed is that in landscape orientation, something weird happens: everything seems to turn sideways when I open the action sheet, as though it's trying to go back to a portrait orientation. Then, when I close the action sheet, things turn right-side-up again but this seems perfectly fine for android device here is an video for that

Android.Emulator.-.Pixel_3_API_29_5554.2021-02-20.14-53-21.mp4

@chrisbobbe I hope I have cleared all the points can you plz help in solving CI failures on CZO here

[chrisbobbe]

Thanks, @abhi0504!

Another thing that you have noticed is that in landscape orientation, something weird happens: everything seems to turn sideways when I open the action sheet, as though it's trying to go back to a portrait orientation. Then, when I close the action sheet, things turn right-side-up again but this seems perfectly fine for android device here is an video for that

Ah, good to know this problem doesn't happen on Android. We still need to either debug the issue on iOS, or just decide not to use this new UI on iOS 😉. I currently prefer the latter (keeping the current UI on iOS), for a few reasons:

• This bug
• I'm not sure this new UI is an improvement on iOS, as I mentioned in messageActionSheet : using Modal instead of @expo/react-native-action-sheet #4490 (comment).
• Black screen appears when user click on Share option. #4407, the black-screen issue, doesn't seem to happen on iOS (I've just tested)
• You might not happen to have a Mac computer, to run Xcode, which is required to test on iOS

So, if you're still interested in the new UI on Android, it'll be necessary to write some conditional logic (using Platform) so that we can keep the current UI on iOS. Unfortunately, this area of the code is already tricky, but it's also quite important, so we should try not to make the code harder to understand than it is now.

That'll likely mean several NFC commits, to refactor the code into a state where adding such a conditional makes sense. In particular, I'm imagining an approach like this:

• Write code to implement the new UI, but don't call it yet
• Refactor the show-action-sheet logic so it uses a simple conditional, e.g., Platform.OS === 'android' ? … : …, but both branches of the conditional call the code for the current UI.

Then, after that,

• A small, simple commit that changes the true branch of the platform-is-Android conditional, so that it calls the code for the new UI.

That way, we can maintain the invariant that no commits have the new UI active on iOS. Since we know we don't want it right now (for the reasons I mentioned, like because it's buggy), that'll be really helpful for the code review. 🙂

One place the platform-is-Android conditional might go is in showActionSheet in src/message/messageActionSheet.js. The iOS branch could call showActionSheetWithOptions, and the Android branch would (eventually) call the function that sets the new-UI modal to be visible, named handleLongPressModal in this revision.

I see that handleLongPressModal sets a piece of state that lives in ChatScreen. Instead, let's have that state live in a new wrapper component that lives in src/webview/MessageList.js—the ChatScreen shouldn't have to care how the message-action-sheet operates. (Also, leaving MessageList's callers to manage the modal state means that state-management logic has to be copied around each caller. So, while the modal works on ChatScreen, it doesn't work on SearchMessagesCard in this revision, since it wasn't copied to there.)

[chrisbobbe]

I see that handleLongPressModal sets a piece of state that lives in ChatScreen. Instead, let's have that state live in a new wrapper component that lives in src/webview/MessageList.js

You might find it helpful to have the MessageList's props get properly type-checked before working on that, though. I've got #4465 open for that.

[chrisbobbe]

Also, it's important to note—the approach I've outlined above has more complexity than we'd expect a new contributor to handle (as mentioned, this area of the code is tricky, but it also deserves lots of attention to correctness and readability). You may find it more pleasant and educational to choose something else to work on; it's up to you. 🙂 I do appreciate your efforts so far, though; it's certainly reminded me that we could be friendlier to new contributors by making this code easier to work with.

[abhi0504]

Thanks @chrisbobbe for your suggestion and your opinion love to know that you appreciate my efforts 😇 and I also think that this area of the code is already tricky, but it's also quite important, so we should try not to make the code harder to understand than it is now and we could be friendlier to new contributors by making this code easier to work with so I think I should leave this PR for now for future refactoring but once again thank you so much @chrisbobbe and @gnprice for your constant help and support 😊

[gnprice]

#4407 has now been fixed in Expo upstream (thanks @abhi0504 for making that upstream bug report!).

It's possible we'll eventually want to switch anyway to something like Modal where we can have more control, but from this thread it looks like there will be some tricky things to get right in order to do so. We'll make a new PR if we end up tackling that at some point.