search

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.12k stars 2.62k forks source link

[HOLD for payment 2024-05-20] iOS - Wallet - Missing up and down key and Done button in the header of account selection list #40126

Closed lanitochka17 closed 1 month ago

lanitochka17 commented 3 months 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!

Version Number: 1.4.62-3 Reproducible in staging?: Y Reproducible in production?: N If this was caught during regression testing, add the test name, ID and link from TestRail: N/A Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch New Expensify app
  2. Go to Profile > Wallet
  3. Add a personal bank account
  4. After returning to ND from Plaid, note the header in account selection list

Expected Result:

There will be up and down button and the word "Done" in the header in account selection list (screenshot from 1.4.61-8)

Actual Result:

The header in account selection list is empty

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/e28ea7bb-2369-4dcd-a9f7-e5294e40f206

Bug6445965_1712846010774!1 4 62-3

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @kevinksullivan
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @luacmartins (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 3 months ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
lanitochka17 commented 3 months ago

@luacmartins FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 3 months ago

We think that this bug might be related to #wave-collect - Выпуск 1

lanitochka17 commented 3 months ago

Production:

https://github.com/Expensify/App/assets/78819774/f90e5414-57a6-4448-8c60-3898b8aeeacf

luacmartins commented 3 months ago

I think we can demote this to NAB and make it external, since the functionality is not broken.

melvin-bot[bot] commented 3 months ago

Unable to auto-create job on Upwork. The BZ team member should create it manually for this issue.

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @kevinksullivan (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

Krishna2323 commented 3 months ago

Proposal

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

iOS - Wallet - Missing up and down key and Done button in the header of account selection list

What is the root cause of that problem?

I tried to debug and find the root cause but couldn't locate it. However, during element inspection, I noticed that modalViewMiddle had a zIndex of 2, and updating it to -1 or 0 resolved the issue.

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

We can update the zIndex of modalViewMiddle here: https://github.com/Expensify/App/blob/8ae5491b7decddaa4102850aaf89e19e2e5adf34/src/styles/index.ts#L785-L787

What alternative solutions did you explore? (Optional)

We can hide the modalViewMiddle using InputAccessoryView or we can add functionality to up/down arrow keys using onUpArrow & onDownArrow

Examples: https://snack.expo.dev/@lfkwtz/react-native-picker-select?platform=ios

If we want we can only hide up/down arrows by applying styles to chevronContainer.

chevronContainer: {
    pointerEvents: 'none',
    opacity: 0,
},
ZhenjaHorbach commented 3 months ago

Proposal

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

iOS - Wallet - Missing up and down key and Done button in the header of account selection list

What is the root cause of that problem?

This problem is relevant for ALL pickers

The main problem is that zIndex:2 was added to the picker header Because it fixed one bug

https://github.com/lawnstarter/react-native-picker-select/blob/master/src/styles.js#L23 https://github.com/lawnstarter/react-native-picker-select/issues/209

But now it causes a problem with the display of the element Since looking at this topic, I concluded that the new architecture sometimes does not work correctly with zIndex

https://github.com/facebook/react-native/issues/38513

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

To fix this issue we can add position: 'relative' for modalViewMiddle to zIndex works correctly

This will save us from potential problems, which is associated with the reason for adding zIndex But it will also allow us to display the header correctly

Screenshot 2024-04-11 at 21 02 16

Plus if we want we can update logic for up and down arrows or hide them using props from documentation

https://github.com/lawnstarter/react-native-picker-select?tab=readme-ov-file#props

What alternative solutions did you explore? (Optional)

NA

MrMuzyk commented 2 months ago

I am Michał from Callstack - expert contributor group. I’d like to work on this job.

I see one effective alternative solution straight away here but we would need someone from UX team to speak on that. A while ago we've refactored BA flow where we replaced this picker with a list of radio buttons. I can see in the codebase that we're already using this radio buttons list in 2 of 3 places if I'm not mistaken and Wallet is the only place left where we're using the old picker component. What could be done is we could drop the picker completely and move to this new design of list of radio buttons. Posting a screenshot of how it could look like below -

image
luacmartins commented 2 months ago

@thesahindia did you get a chance to look at the proposals above?

thesahindia commented 2 months ago

I think we should ask the design team for their opinion about https://github.com/Expensify/App/issues/40126#issuecomment-2051203304

ZhenjaHorbach commented 2 months ago

I think we need to fix this bug anyway Because this problem is relevant for ALL pickers (Not only the account selection list)

MrMuzyk commented 2 months ago

I think we need to fix this bug anyway Because this problem is relevant for ALL pickers (Not only the account selection list)

If the issue exists for all picker then I agree - we should apply a fix for that too. Maybe it is also worth considering dropping usage of them completely - after all they have been dropped in other places. At the moment counting this one in wallet we have 3 usages of <Picker/>

luacmartins commented 2 months ago

That makes sense and we should probably update those usages. Do we have screenshots of the before/after UI?

MrMuzyk commented 2 months ago

I've made a few screenshots to showcase the difference

Before After
wallet_before wallet after
footer_before footer_before_2 footer_after

Turns out that there are actually 2 usages and not 3. 3rd usage is here

https://github.com/Expensify/App/blob/21ee1a3772253538bba74b2bc655213dc7bdb68a/src/pages/workspace/reimburse/WorkspaceRateAndUnitPage.tsx#L142

but when I've tried to search for this spot in the actual app I've found out that this page (WorkspaceRateAndUnitPage) is not used anywhere in the code and can be removed alltogether

luacmartins commented 2 months ago

@Expensify/design would love your thoughts on the Picker changes above. I think it's fine on the RHP, but the login page language selection looks odd to me.

dannymcclain commented 2 months ago

I'm a big fan of moving to the radio options instead of the picker in the RHP for this. I honestly didn't even know we had any actual pickers in the product!

The login page/footer language select is pretty rough as radio buttons, so I think we should just leave that one alone for now.

Curious for the other designers' thoughts.

shawnborton commented 2 months ago

Before we go too far with this, I think we're in the process of redoing this entire flow, check this out: https://github.com/Expensify/App/issues/36648

So perhaps we can just close this out as we're changing how we display accounts to be something more like this: image

shawnborton commented 2 months ago

cc @mountiny @koko57

dubielzyk-expensify commented 2 months ago

Agree that the radios are nice, but Shawns comment makes sense

MrMuzyk commented 2 months ago

If this Picker is being changed anyway shouldn't we cover the other two before closing this bug?

  1. Apply the fix language Picker in footer

    image

  2. Remove the unused page (WorkspaceRateAndUnitPage) that has the other Picker. Unless I'm mistaken/this is being covered in some other issue.

luacmartins commented 2 months ago

I'm not sure we're covering that in other issues. I agree that we should update the remaining pickers though. I'll let @thesahindia review the proposals before continuing.

mountiny commented 2 months ago

I think I agree with @shawnborton that we could cover this in the wallet flow refactor, but also since there are other places where we should fix it, we might as well just fix it everywhere here.

luacmartins commented 2 months ago

I think we can keep it in this issue since we have other places that need to be updated.

melvin-bot[bot] commented 2 months ago

@luacmartins @kevinksullivan @thesahindia this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

thesahindia commented 2 months ago

I think we can keep it in this issue since we have other places that need to be updated.

The final decision is to replace all the pickers, right?

luacmartins commented 2 months ago

Correct, except for the ones on the login page

thesahindia commented 2 months ago

Waiting for updated proposals!

MrMuzyk commented 2 months ago

Proposal

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

iOS - Wallet - Missing up and down key and Done button in the header of account selection list

What is the root cause of that problem?

Picker header has zIndex 2 that causes other Picker elements to display incorrectly. On top of that, it has an outdated design that should be replaced everywhere except for footer

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

What alternative solutions did you explore? (Optional)

N/A

kevinksullivan commented 2 months ago

Adding to vip-split

luacmartins commented 2 months ago

@MrMuzyk your proposal looks good.

MrMuzyk commented 2 months ago

I will implement this once I'm back after long weekend I'm starting today (will be back 06.05)

melvin-bot[bot] commented 2 months ago

@luacmartins, @kevinksullivan, @MrMuzyk, @thesahindia Whoops! This issue is 2 days overdue. Let's get this updated quick!

melvin-bot[bot] commented 1 month ago

Reviewing label has been removed, please complete the "BugZero Checklist".

melvin-bot[bot] commented 1 month ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.72-1 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

If no regressions arise, payment will be issued on 2024-05-20. :confetti_ball:

For reference, here are some details about the assignees on this issue:

melvin-bot[bot] commented 1 month ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

melvin-bot[bot] commented 1 month ago

Payment Summary

[Upwork Job]()

BugZero Checklist (@kevinksullivan)

kevinksullivan commented 1 month ago

Confirmed summary is accurate. @thesahindia can you finish out the checklist above?

thesahindia commented 1 month ago

Not a regression. It was a minor issue and we don't need a test case for this.

melvin-bot[bot] commented 1 month ago

@luacmartins, @kevinksullivan, @MrMuzyk, @thesahindia Huh... This is 4 days overdue. Who can take care of this?

JmillsExpensify commented 1 month ago

$250 approved for @thesahindia

melvin-bot[bot] commented 1 month ago

@luacmartins, @kevinksullivan, @MrMuzyk, @thesahindia 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

luacmartins commented 1 month ago

Are we good to close this one?

thesahindia commented 1 month ago

Yeah, we can close this.

luacmartins commented 1 month ago

Awesome! Thanks everyone!