[$2000] iPad - UI Inconsistency of the popover menus - reported by @Santhosh-Sellavel

[mvtglobally]

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

---

Action Performed:

1. Open app on a iPad
2. Navigate to "+"
3. The Popover menu shows on the left side of the screen

Expected Result:

On wide iPad devices, the popover menu is shown at the bottom left of the screen

Actual Result:

On wide iPad devices, the popover menu should show inline with the current modal on the right side of the screen, just as web and Desktop does:

Additional areas where issue is occurring

• Compose Attachment options
• Call Options
• Message Context Options
• Workspace ( Three dots) More Menu Options
• Add Payment Options
• Add/Edit Photo Options for Workspace & User Profile.
• Transfer Balance Options

Workaround:

unknown

Platform:

Where is this issue occurring?

• iOS

Version Number: 1.1.32-0
Reproducible in staging?: Y
Reproducible in production?: Y
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Simulator.Screen.Recording.-.iPad.Pro.11-inch.3rd.generation.-.2022-01-04.at.22.26.20.mp4

Expensify/Expensify Issue URL:
Issue reported by: @Santhosh-Sellavel
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1641315894001600

View all open jobs on GitHub

[MelvinBot]

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

[MelvinBot]

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

[Gonals]

Setting labels and sending to the pool!

[srdjanrist]

Proposal

Add prop fromSidebarMediumScreen={!this.props.isSmallScreenWidth} to all Popover components, wherever the issue was noticed.
E.g. ReportActionCompose (line 564)

This way, on iPad (which has width around 834px) it will behave as on browser, since it falls in category of medium screens.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[MelvinBot]

This issue has not been updated in over 14 days. eroding to Weekly issue.

[SofiedeVreese]

Oh shit this one slipped through the cracks bc I was unassigned. @Gonals did you want this pushed externally?

If so I'll assign myself.

[Julesssss]

@parasharrajat hmm, yes that is a great point. So I believe we would expect the following:

[parasharrajat]

Yeah for all menus. For example, add attachment menu as well.

[Santhosh-Sellavel]

Yes, that would be the ideal solution!

[aitoraznar]

Hi. How do you usually proceed with this?. You develop it internally? Create a new ticket? This issue is still valid?

[parasharrajat]

The issue is still valid and open for proposals. @Julesssss Can you please update the Expected result section?

[aitoraznar]

Proposal

Some changes need to be made in some components: Popover.native to set as Popover in medium devices (Ipad/tablets), And calculate the popover position in AttachmentPicker, ReportActionCompose and AddPaymentMethodMenu.

Some areas such as Call Options and Add/Edit Photo Options for User Profile don't need any code changes. Will check other areas if my proposal is accepted.

Video of my working branch
https://user-images.githubusercontent.com/2470879/201991522-b0550ad0-7b0a-4a47-a650-7eb4d18d78b6.mp4

[parasharrajat]

@aitoraznar I need more details for considering your proposals. Though, I have a question for you.

How will you tackle the dynamic positioning?
To test this:

1. Go to Settings> payments on staging (staging.new.expensify.com).
2. Add a bank account. (make sure you are in the plaid sandbox, otherwise, set use Staging server from the preferences page)
3. Then open the app on Tablet|iPAD.
4. login with the same account.
5. Go to payments page.
6. Click on any added payment method.

[aitoraznar]

Hi. The positioning is relative to the button, so working well on landscape.

How I can add a dummy Bank account? I'm pretty new to the project, tons of documentation...

Many thanks

[parasharrajat]

If you follow the step above you should be good with it. Notice I mentioned adding the account from the web app as Native currently uses PROD API. Plaid flow is pretty easy just keep continue to next steps.

let me know if you are stuck at some step.

[aitoraznar]

Hi @parasharrajat. Any idea why the "getClickedTargetLocation" method would not work properly on Native events? It is not getting the position of the clicked element.

Any related issue apart from https://github.com/Expensify/App/issues?q=getClickedTargetLocation ?

Finally added a Paypal account to test the Payment methods.

thanks

[parasharrajat]

It is not implemented on native https://github.com/Expensify/App/blob/main/src/libs/getClickedTargetLocation/index.native.js

[aitoraznar]

Hi @parasharrajat.

** Proposal update**

I implemented the "getClickedTargetLocation" using the "measureInWindow" method used in other parts of the app.

Video of the implementation
https://user-images.githubusercontent.com/2470879/202152733-6a75b90a-370c-4e4e-a3af-610bdb83f2d3.mp4

Look at the render blink when pressing "change profile photo", only happens on landscape. Any clue about this?

Many thanks

[parasharrajat]

No idea, I am expecting a solution via proposals.

1. How will you reduce the delay that is taken for the menu to open?
2. There is a small flicker when we open attachment menu on composer, how will you solve it?

[aitoraznar]

Hi @parasharrajat.

About your questions:

1. How will you reduce the delay that is taken for the menu to open?

I don't see what are you referring to. Can you say to me in which exact second of the video is happening?

2. There is a small flicker when we open attachment menu on composer, how will you solve it?

Fixed. It was the position being used from an old state when opening different popovers on that page. To reproduce it we had to press the "Add Payment" button and then "Existent Payment" button.

Updated video
https://user-images.githubusercontent.com/2470879/202235862-3834ee90-3eae-4fb9-9645-6d2b7dbbc201.mp4

Many thanks

[parasharrajat]

Can you say to me in which exact second of the video is happening?

0.27 on the first video. It takes a bit of time to open as the measure takes time to resolve and it is async.

Can you please lay down the changes you would do?

[aitoraznar]

Hi. The button works fine with the latest changes.

Proposal

• Change the modal type from "BOTTOM_DOCKER" to "POPOVER" for medium/large native devices
• Implement "getClickedTargetLocation" method for native devices
• Position the "Message Context Options" popup
• Position the "Add attachment" popup
• Position the "Add Payment Options" popup
• Position the "Avatar photo " popup for Workspace & User Profile
• Position the "Transfer Balance Options" popup
• No changes in "Call Options" popup
• No changes in "Workspace ( Three dots) More Menu Options" popup

Many thanks

[parasharrajat]

It would help us if you can explain the changes you would do to the app for this issue. You have still not explained how will you solve the challenges that I mentioned above.

Links:

• CONTRIBUTING Guidelines
• App overview and local development
• CODE OF CONDUCT
• Code style and patterns
• Styling Components

I would recommend checking out some previous jobs, which already got a contributor assigned to see how they posted the proposals. There is also this part in our contributor guidelines which you might find useful.

Please, also share the exact changes in code you had to make to fix this issue. You can see from all the other issues and contributors that we won't just take the code and run away, no need to worry about that :)

Thank you!

[aitoraznar]

Hi. Same steps as the last message but with code.

Proposal

1. Change the modal type from "BOTTOM_DOCKER" to "POPOVER" for medium/large native devices

2. Implement "getClickedTargetLocation" method for native devices

3. Position the "Message Context Options" popup
   

4. Position the "Add attachment" popup

5. Position the "Add Payment Options" popup
   

6. Position the "Avatar photo " popup for Workspace & User Profile

7. Position the "Transfer Balance Options" popup
   

Many thanks

[parasharrajat]

The code is looking good. As I worked on this task earlier, there were a few challenges.

We are using measureInWindow in Implement "getClickedTargetLocation" method for native devices. It delays the menu opening after you click the button. You can also see this in your videos. How will you solve this?

this is where I got stuck earlier.

[aitoraznar]

Hi @parasharrajat. I don't came up with an alternative to measureInWindow.
I suggest to calculate the Popup positions on the background once the page is loaded. This would not affect the page time load and the position will be already calculated before he user try to press the buttons.

[parasharrajat]

I suggest to calculate the Popup positions on the background once the page is loaded.

Ok. What about the menus of dynamic anchors? Let's say the anchor move position. This can be observed on the payment method list. I onLayout does not work well. It does not fire all the time in Animatable Views.

[JmillsExpensify]

@Julesssss To help clear out the extensive backlog of /App bugs, we're putting the spotlight on all bugs older than 4 weeks old. To help unblock the roadmap and get our bug pipeline back in equilibrium, can you:

• Decide whether any proposals currently meet our guidelines and can be approved as-is
• For any that can't, please take this issue internal and treat it as one of your highest priorities
• If you have any questions, don't hesitate to start a discussion in #bug-zero

Thanks everyone!

[aitoraznar]

Hi @parasharrajat, I was investigating about your suggestion.
Some buttons are created inside components with the only responsibility to render them. We need to propagate somehow the list of buttons to the parent containing the Popover, creating a Context of more callbacks.
We would have to made a lot of changes to be able to pre-calculate positions on dynamic buttons.
I think It will mess the code and the code will be less maintainable if we do that.

Using measureInWindow we have that little trade off of the Popover lasting a little longer to show. Do you think the current speed is enough to have a responsive UX?

Video with the current speed:
https://user-images.githubusercontent.com/2470879/202750481-90473de6-31aa-468b-966d-77fa16747f5e.mp4

[JmillsExpensify]

Based on our discussion in the community, I'm going to close this issue. Given that tablets are not an officially supported platform, this issue is more accurately a new feature request than a bug. Closing.

[aitoraznar]

Hi @parasharrajat. Can you update to me the status of the issue?. You said to me this issue was still valid and It wasn't to be developed internally.

Many thanks.

[parasharrajat]

It was valid when I mentioned but internal priorities can change.

Recently, we are focusing on a major milestone of the project. Due to there being many issues with iPad and Tablets which aren't focusing, we decided to postpone all issues related to Tablets and iPad.

We may reopen this in future but currently no one is working on this.