Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.32k stars 2.76k forks source link

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

Closed mvtglobally closed 1 year ago

mvtglobally commented 2 years ago

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:

Screenshot_1668176045

Additional areas where issue is occurring

Workaround:

unknown

Platform:

Where is this issue occurring?

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 Screen Shot 2022-01-24 at 10 42 24 PM

https://user-images.githubusercontent.com/43995119/150906959-44d501c9-3428-494f-bc9a-8f11d602c5e8.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 commented 2 years ago

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

MelvinBot commented 2 years ago

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

Gonals commented 2 years ago

Setting labels and sending to the pool!

srdjanrist commented 2 years ago

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 commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

MelvinBot commented 2 years ago

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

SofiedeVreese commented 2 years ago

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.

Gonals commented 2 years ago

Ah damn! Yeah, I think this is a good candidate for external

SofiedeVreese commented 2 years ago

Assigning myself as External.

SofiedeVreese commented 2 years ago

Posted on Upwork: https://www.upwork.com/jobs/~0112ee5bb8a9f27309

MelvinBot commented 2 years ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @parasharrajat (Exported)

MelvinBot commented 2 years ago

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

parasharrajat commented 2 years ago

@srdjanrist I don't understand the purpose of your proposed change. Could you please explain what will this prop do?

jeromekessler26 commented 2 years ago

Hello,

Not seeing code it is hard to see a problem and fix it but I do have a couple of possible solutions. Try setting the button width to Dimensions.get('window').width. Another solution is creating a pressable with a view component inside. Make the view component the size you want. Then when you click the pressable change the view's background color. I would love to help but without code it's quite hard.

parasharrajat commented 2 years ago

Hello @jkgaming88 ,

It is OSS and you can head over to Read.me for more..

jeromekessler26 commented 2 years ago

Hello @parasharrajat

Fixed the problem. Change width in popoverMenuItem style to Dimensions.get('window').width. The '100%' was causing the issue. This makes the menu item the size of the modal. It will never exceed it. You could set the value to 10,000 and it will still work. I think React Native is thinking 100% width is the size of the initial pop over. This fixes that

https://user-images.githubusercontent.com/79109856/155439735-c7c61a18-7e33-4c89-b8ae-347cd3688c3f.mp4

jeromekessler26 commented 2 years ago

screenshot

This is the initial popover I meantioned before. Notice how the width of the popover is the same as the button pre-fix. I do not know why it happened but that is the reason.

srdjanrist commented 2 years ago

@srdjanrist I don't understand the purpose of your proposed change. Could you please explain what will this prop do?

Purpose of fromSidebarMediumScreen is to decide which type of modal will be shown. If fromSidebarMediumScreen = true POPOVER modal will be shown. If fromSidebarMediumScreen = false DOCKED modal will be shown.

As we can see on iPad 11 inch, in the side menu, current behavior is to show popover, instead of docked modal. (behavior is similar to what can be seen on web). Also, on the same device, we can see that the screen is split, meaning it shows sidemenu with other reports, and on the right, current report is shown. (again behavior similar to web).

By setting the flag fromSidebarMediumScreen to what I have proposed, we will have consistent behavior, where it will always show popover, instead of docked modal.

This can be seen on the following images.

Current behavior:

1-side-menu-current-behavior 2-action-compose-menu-items 3-add-attachment

Proposed fixed behavior:

4-action-compose-fixed 5-action-compose-attachment-fixed
parasharrajat commented 2 years ago

Ok Sounds good @srdjanrist . I like @srdjanrist 's proposal.

cc: @Julesssss

:ribbon: :eyes: :ribbon: C+ reviewed

SofiedeVreese commented 2 years ago

fyi @Julesssss let me know if you're happy to proceed with @srdjanrist 's proposal!

Julesssss commented 2 years ago

Thanks all, yep the proposal sounds good.

MelvinBot commented 2 years ago

📣 @srdjanrist You have been assigned to this job by @Julesssss! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

SofiedeVreese commented 2 years ago

Perfect, thanks @Julesssss .

@srdjanrist I've just hired you in Upwork!

SofiedeVreese commented 2 years ago

Not overdue Melvin, we have a PR that's being worked on.

SofiedeVreese commented 2 years ago

Reassigning CM to @kadiealexander.

This one has an open PR.

parasharrajat commented 2 years ago

@srdjanrist Could you please provide an update on PR?

srdjanrist commented 2 years ago

@srdjanrist Could you please provide an update on PR?

Thanks for waiting for me @parasharrajat . I have updated the PR with a few commits. So you can check them.

kadiealexander commented 2 years ago

@srdjanrist I can see the last comment on the PR was 5 days ago, could you please leave a comment here letting us know where you're up to?

srdjanrist commented 2 years ago

@srdjanrist I can see the last comment on the PR was 5 days ago, could you please leave a comment here letting us know where you're up to?

Thank you for the heads up. I will check those few things mentioned in the PR today (28 March).

srdjanrist commented 2 years ago

Okay, seems like we're seeking a new contributor here. Open to proposals again!

PR is up. @parasharrajat is currently testing.

https://github.com/Expensify/App/pull/7985#issuecomment-1089237746

parasharrajat commented 2 years ago

Yup. Review would be done today.

Julesssss commented 2 years ago

Bumping this. Seems like we're at the review stage?

kadiealexander commented 2 years ago

@srdjanrist could you please provide an update for us here?

mallenexpensify commented 2 years ago

Looks like the PR is still moving along

melvin-bot[bot] commented 2 years ago

A Contributor Manager will be assigned to issue payment via Upwork if we deploy an associated Pull Request to production. Per Contributing.md.

Julesssss commented 2 years ago

Awaiting changes based on feedback, I bumped one of the unresolved threads

srdjanrist commented 2 years ago

Won’t be able to continue working on the proposed changes. Someone can take over. Thank you

Get Outlook for iOShttps://aka.ms/o0ukef


From: Jules Rosser @.> Sent: Tuesday, May 3, 2022 11:25:16 AM To: Expensify/App @.> Cc: srdjanrist @.>; Mention @.> Subject: Re: [Expensify/App] iPad - UI Inconsistency of the popover menus - reported by @Santhosh-Sellavel (Issue #7375)

Awaiting changes based on feedback, I bumped one of the unresolved threads

— Reply to this email directly, view it on GitHubhttps://github.com/Expensify/App/issues/7375#issuecomment-1115897968, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AGBXC4RLLM4WDFBDIG67PULVIDWHZANCNFSM5MXEGXYA. You are receiving this because you were mentioned.Message ID: @.***>

Julesssss commented 2 years ago

Hi @srdjanrist, sorry to hear that. Is it because of the amount of time/effort taken? Because if so, the job prices can always be raised to match the effort.

srdjanrist commented 2 years ago

Hey @Julesssss thank you for the response. It has a little bit to do with time involved so far. Main reason is that I can't commit myself for long periods of time and focus on the issue ahead (mainly because I started working on another project, while working on this issue). And due to that, since I am a responsible person, I don't want to drag you along and slow everything down.

Certainly, I would like the code that was already made into a pull request, to be used, so that it doesn't go to waste fully.

I had a really nice plan to work on the issues on Expensify, but because of other obligations those plans didn't really end up well.

Julesssss commented 2 years ago

That totally makes sense, thanks again for your input. We'd love to welcome you back if your situation changes in the future!

Julesssss commented 2 years ago

Taking over the PR

mallenexpensify commented 2 years ago

Let's keep @parasharrajat assigned for PR review, he'll be compensated in full for the C+ job