FAB menu - the menus for FAB menu slide in from the left-to-right, instead of bottom-to-top

[isagoico]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

Upwork Job - https://www.upwork.com/jobs/~0156d103876d9e96f7

---

Expected Result:

Menu should slide from bottom to top

Actual Result:

Menu slides from left-to-right

Action Performed:

1. Open e.cash app on iOS or aOS
2. Log in and click on the FAB button.

Workaround:

No need, visual issue.

Platform:

Where is this issue occurring?

Web
iOS ✔️
Android ✔️
Desktop App
Mobile Web

Version Number: 1.0.62-0

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

WhatsApp.Video.2021-06-03.at.12.47.55.PM.mp4

onal supporting documentation

Expensify/Expensify Issue URL:

View all open jobs on Upwork

---

From @roryabraham https://expensify.slack.com/archives/C01GTK53T8Q/p1622681730473400

On mobile platforms, the menus for FAB menu slide in from the left-to-right, instead of bottom-to-top

[MelvinBot]

Triggered auto assignment to @HorusGoul (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

[roryabraham]

I think this can be external. If you agree @HorusGoul then the next step would be for you to:

1. Remove your assignment (unless you are a member of the engineering-contributor-management team)
2. Add the External label to get this auto-assigned to a member of the (non-engineering) contributor management team

[MelvinBot]

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

[kevinksullivan]

upwork job posted

https://www.upwork.com/jobs/~0156d103876d9e96f7

[MelvinBot]

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

[parasharrajat]

Proposal

Explanation

1. the required Behaviour is that Menu should be Bottom_docked on Mobile devices and Popover for the rest.
2. In https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/components/Popover/index.js#L11-L19
   we control this behavior but this file is only active for Web, M-web. Thus on M-web we can't find this issue.
   But https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/components/Popover/index.native.js#L11-L16

We don't have any behavior like this. We just want to Bottom Dock the Menu which already has defined animations thus props from the parent are not needed. That's why we are clearing the props in the next step for Mobile sizes.

Now when we pass the animation Props from SidebarScreen
https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/pages/home/sidebar/SidebarScreen.js#L103
These were meant for the Popover type. M-web, handles it well due to these lines.

{...props}
        animationIn={props.isSmallScreenWidth ? undefined : props.animationIn}
        animationOut={props.isSmallScreenWidth ? undefined : props.animationOut}

But index.native.js 's animation will be overwritten.

Solution

so Solution here is to change the View logic of https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/components/Popover/index.native.js .

TO

 <Modal
        type={CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED}
        // eslint-disable-next-line react/jsx-props-no-spreading
        {...props}
        // Only BOTTOM_DOCKED so we don't need animation props.
        animationIn={undefined}
        animationOut={undefined}
    />

OR We _.omit the animation props from the passed props and then forward it to Modal.

This does not affect any other Menus at all. We originally expected this behavior but forgot to patch native devices with respect to M-web. That's what I proposed as a solution.

[gauravahujame]

Understanding

From SidebarScreen.js, we pass animationIn and animationOut to the CreateMenu component.
https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/pages/home/sidebar/SidebarScreen.js#L103-L104
These are further passed to Popover => Modal => BaseModal. Inside BaseModal, we prioritize animation props over the ones returned from getModalStyles function.
https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/components/Modal/BaseModal.js#L95-L96

Problem

SidebarScreen.js should not send the animations because they can be different based on the platform. So it does not make sense to send fadeInLeft and fadeOutLeft from here because they don't apply to mobile devices.

Solution

• To remove animationIn and animationOut props from SidebarScreen.
• It sounds appropriate for CreateMenu to decide animations since menus have platform specific animations.
• For mobile devices, we can make the following change inside https://github.com/Expensify/Expensify.cash/blob/3eee51b9c4af23d04845468df227e94fc0034f0a/src/components/CreateMenu/index.native.js

            <BaseCreateMenu
                // eslint-disable-next-line react/jsx-props-no-spreading
                {...this.props}
                onMenuHide={this.onMenuHide}
                onItemSelected={item => this.selectItem(item)}
                animationIn={this.props.animationIn || 'slideInUp'} // THIS
                animationOut={this.props.animationOut || 'slideOutDown'} // THIS
            />

With this, menus have a platform specific default animation and also the flexibility of overriding them if needed in future.

Also, @parasharrajat I think we should let PopupOver to relay animation props from parent components instead of setting them undefined. This way it won't affect other parts of the app which are using PopOver.

Update: I've been able to test my solution and this is how it looks
https://user-images.githubusercontent.com/26275462/120879414-c7c73700-c5e0-11eb-9594-e308ca773cf2.mp4

Screen.Recording.2021-06-05.at.10.03.57.AM.mov

[amnan181]

Proposal

You just have to change the animation type to work as you want

1. You have to add the initial animation type like animationIn="slideInUp"
2. Same as first one you have to change the animation out animation which will be triggered when your modal is going to be closed so simply add the type which type of behaviour you want while closing the modal animationOut="slideOutDown"

Thanks!

[dharammachra]

@isagoico @HorusGoul -

Issue -

SidebarScreen.js - We have hard coded value
animationIn="fadeInLeft"
animationOut="fadeOutLeft"

It can be different based on the platform.

Proposal -
We need to remove hard coded animationIn and animationOut props from SidebarScreen.

1. We can simply create a helper method getAnimationInType() with the following structures
   .
   └── libs/
   └── getAnimationInType/
   ├── index.js => 'fadeInLeft'
   └── index.native.js - return () =>’ slideInUp’

Then we can simply assign the value -

animationIn ={getAnimationInType()}
'
OR

2. We should simply check the platform specific animations in the index.native.js

src/components/CreateMenu/index.native.js
animationIn={this.props.animationIn || 'slideInUp'}
animationOut={this.props.animationOut || 'slideOutDown'}

Please find the attached video for Mobile and Web after my solution:

1. Mobile -
   https://user-images.githubusercontent.com/368119/120922094-deef4d00-c6e4-11eb-8772-67104223c994.mov

2. Web -

Web.mov

[Luke9389]

I don't understand why this is happening. This menu should be getting set as type=CONST.MODAL.MODAL_TYPE.BOTTOM_DOCKED. Do any of you see why this problem is occurring? I'm not asking for altered proposals at this time, just an explanation of what's going on.

Edit: I see it now. Talked 1:1 with @roryabraham.

[Luke9389]

This issue was discussed internally and it was decided that we'd use a different solution that has not been mentioned above. Thanks for you're time folks!