[$1000] Add animation when the emoji picker is closed

[mountiny]

Problem

On mobile, the emoji picker popover is not being closed with animation compared to other popovers.

This is border line bug and new improvement, it feel like inconcsistency bug

Why is it important

This is not consistent UI

Solution

Lets add an animation to this popover when its being closed

Comign from https://expensify.slack.com/archives/C01GTK53T8Q/p1682013158901689

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010b1720ceac20d4b1
• Upwork Job ID: 1651348863138013184
• Last Price Increase: 2023-05-17

[MelvinBot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[MelvinBot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[mountiny]

@esh-g you have raised it

[MelvinBot]

Job added to Upwork: https://www.upwork.com/jobs/~010b1720ceac20d4b1

[MelvinBot]

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

[MelvinBot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @aimane-chnaif (External)

[MelvinBot]

Triggered auto assignment to @arosiclair (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[alexpensify]

Since this was discussed in Slack, I've assigned External

[kmorales13]

Proposal

Please re-state the problem that we are trying to solve in this issue.

On mobile, the emoji picker popover is not being closed with animation compared to other popovers.

What is the root cause of that problem?

Currently the animationOutTiming prop is set to 1ms.

What changes do you think we should make in order to solve the problem?

We would need to remove/update this line here

App/src/pages/home/report/ContextMenu/PopoverReportActionContextMenu.js

Line 309 in 9ec7704

animationOutTiming={1}

We would also need to verify there's no performance impact, since I'm guessing that's the reason this was set to 1 in the first place.

[mountiny]

@aimane-chnaif @kmorales13 what kind of animation library is providing the animations, I think we would like to move everything to Reanimated 3 is possible. That coul dbe part of this issue since otherwise its a simple change

[kmorales13]

@aimane-chnaif @kmorales13 what kind of animation library is providing the animations, I think we would like to move everything to Reanimated 3 is possible. That coul dbe part of this issue since otherwise its a simple change

The library used for Popovers/Modals, react-native-modal, uses react-native-animatable internally for animations

[aimane-chnaif]

@mountiny should we give priority to @esh-g since they suggested P/S and researched enough time? Here's slack convo

[mountiny]

@aimane-chnaif I think its same as a bug, in this case so not necessarily if we can find as good solution from other contributors.

[esh-g]

Please note that the solution that @kmorales13 is suggesting was already suggested by me in the slack conversation (https://expensify.slack.com/archives/C01GTK53T8Q/p1681233318285209).

Since this was a one-line change, I didn't post a Proposal yet as I was testing for better performance and accommodation of the animation for EmojiPicker as well. Because if you see in the video below, having animation on the contextMenu but not the EmojiPicker can look a bit odd. (As also suggested by the name of the issue)

Screen.Recording.2023-04-27.at.1.18.04.PM.mov

But even when we enable the animation for EmojiPicker, due to laggy performance, the animations are barely noticeable.

Screen.Recording.2023-04-27.at.1.27.49.PM.mov

The solution is to use the nativeDriver which can help massively with the performance.

Screen.Recording.2023-04-27.at.1.29.58.PM.mov

But it also causes the contextMenu to flash before opening the EmojiPicker. So, after I am able to solve that, I'll post a proposal here for both contextMenu and EmojiPicker

cc @mountiny @aimane-chnaif

[aimane-chnaif]

@esh-g ok good, we won't assign anyone until we 1000% confirm that it won't cause any performance issue or any other regression.

[mountiny]

Interesting, I would like to hear from SWM if they can think of anything which might have better performance/ reliability for our needs here instead of the react-native-modal

[mountiny]

Thanks for raising that @esh-g I have also asked SWM if they have any suggestions how to improve this performance or maybe even use a different library for this popover which would use Reanimated for hopefully better performance

[esh-g]

@mountiny Should we just create a fork of react-native-modal with reanimated? We already are using a patch in the library and maybe this can help us prevent unwanted updates if we maintain it ourselves?

[mountiny]

We are really trying to avoid any forks, lets wait for what SWM will say about this.

[mountiny]

@esh-g ok let's continue with your direction, have you had any luck resolving the flickering issue?

[alexpensify]

@mountiny should we mark this as external and assign it to @esh-g or wait for more proposals here?

[aimane-chnaif]

3 options:

• still research for iOS modal native animation to be compatible with our app
• use js animation for now (native animation is only applied to android - current app)
• just close the issue if this is not worthy of investigating and fixing

I suggest 3rd option given priority to more urgent issues. What do others think?

[mountiny]

I think we also need to aim for cross platform alignment, if there is animation in android there should be same in iOS so I think we should explore the animations in iOS, there must be a good solution for this.

We can get SWM, reanimated experts to help with this too if we define clearly what the problem is here.

[aimane-chnaif]

Good idea to export this to animation experts.
But react-native-modal uses react-native-animatable, while react-native-reanimated is SWM library

[mountiny]

I know but they could maybe propose changes in general to make this flow better. I think we prefer to move to something which uses reanimated.

@esh-g what do you think?

To confirm, we have reverted just the iOS solution, so this is implemented for Android right?

[aimane-chnaif]

To confirm, we have reverted just the iOS solution, so this is implemented for Android right?

Right, all keep same before creating this GH.

[esh-g]

@esh-g what do you think?

Well I think we should still seek re-animated as an option.. I even suggested a library here before which maybe useful in this scenario. But I will have to do some experimenting..

[alexpensify]

@mountiny - Thank you for the update, should we change this one back to Daily?

[alexpensify]

I'm going to make this a daily again since the solution was reverted.

[mountiny]

I think we can treat this as a new feature at this point

[melvin-bot]

Current assignee @alexpensify is eligible for the NewFeature assigner, not assigning anyone new.

[mountiny]

SWM is going to look at the options for the modal

[alexpensify]

Thank you for the update @mountiny!

[alexpensify]

No update

[esh-g]

@mountiny @aimane-chnaif
Hey, checkout what I have been working on:

Screen.Recording.2023-06-05.at.7.02.33.PM.mov

I am using the gorhom/bottom-sheet library here (with reanimated).
This is of course still experimental but let me know if you like it and I should continue refining it.
I think we can use this for the native platforms and the context menu can remain using react-native-modal library. We reverted that due to the issue of state updating before the modal was hidden but that can be solved easily as mentioned here: #17982 (comment)

[esh-g]

Here is a version with dynamic height adjustment and footer position:

Screen.Recording.2023-06-05.at.7.57.30.PM-1.mov

[wojtus7]

@esh-g looking good!
I think using gorhom/bottom-sheet is a great solution for this issue. Well documented and actively maintained!

[mountiny]

I think this would require a separate Problem/Solution in the open source channel @esh-g are you willing to take that on?

[esh-g]

I think this would require a separate Problem/Solution in the open source channel @esh-g are you willing to take that on?

Yeah sure! 👍

[esh-g]

Posted the Problem/Solution here: https://expensify.slack.com/archives/C01GTK53T8Q/p1686042129293829

cc @mountiny

[mountiny]

thanks

[alexpensify]

@mountiny - I reviewed the Slack convo and it looks like this one needs some more tuning. Is that a good summary of where we are at here?

[mountiny]

@esh-g Actually I feel like we got to a freezing point, would you agree we can close this issue for now and come back to this maybe in future once there will be some better library or polish like this will be higher priority?

Is there anything blocking us from closing this issue out?

[alexpensify]

Alright, I see @esh-g 👍🏼, so I'm going to close this one out.