Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] Bank Account - Arrow next to bank account selector is not clickable #35083

Closed
3 of 6 tasks
lanitochka17 opened this issue Jan 24, 2024 · 50 comments
Closed
3 of 6 tasks
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Jan 24, 2024

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.31-3
Reproducible in staging?: Y
**Reproducible in production?:**No, unable to check prod
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #35032

Action Performed:

  1. Log in to New Expensify
  2. Click on profile icon in LHN
  3. Navigate to wallet > Add Bank account > Personal bank account
  4. Select at least 2 accounts in Plaid flow, and finish the flow
  5. Click on arrow pointing downwards next to Account field

Expected Result:

Account dropdown with the list of available bank accounts should open

Actual Result:

Clicking on arrow does not open or close the dropdown

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6353486_1706117422056.2024-01-24_17-14-08.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01eace76bf2cf18311
  • Upwork Job ID: 1752351557704245248
  • Last Price Increase: 2024-01-30
  • Automatic offers:
    • aimane-chnaif | Contributor | 28134305
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Jan 24, 2024
Copy link

melvin-bot bot commented Jan 24, 2024

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

Copy link
Contributor

👋 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.

@neonbhai
Copy link
Contributor

This is an issue with all pickers in the app and has existed before.

Also reproducible on prod with Distance Rate Unit Picker:

Screen.Recording.2024-01-25.at.2.07.17.AM.mov

not a deploy blocker

cc: @Julesssss

@thienlnam thienlnam added Daily KSv2 and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Jan 25, 2024
@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 25, 2024

Proposal

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

Bank Account - Arrow next to bank account selector is not clickable

What is the root cause of that problem?

This is a bug in react-native-picker-select package. lawnstarter/react-native-picker-select#237

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

We can solve it by adding zIndex:-1 to the icon container and by making the RNPickerSelect transparent and applying the background colour to the parent view.

Result

arrow_click.mp4

@dragnoir
Copy link
Contributor

Proposal

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

Arrow not pressable

What is the root cause of that problem?

The issue is within the <Picker> component, the use of the native component <RNPickerSelect> where an issue already exist as mentioned here lawnstarter/react-native-picker-select#237

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

Change the <Picker> component with a new page. When user click on the dropdown, a new page with the list of options will be opened. Like we do for the other options:

2024-01-25.15.34.47.mp4
<MenuItemWithTopDescription
                    title={------}
                    description={props.translate('-------')}
                    shouldShowRightIcon
                    disabled={-------}
                    onPress={() => Navigation.navigate(ROUTES.-------)}
                />

@Krishna2323
Copy link
Contributor

I think we had decided to place the rate & unit input on the same page, thats why we use Picker.

@dragnoir
Copy link
Contributor

Then we need to fix it, it's a bad design on web currently

image

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 26, 2024

Although we can but this issue is not about design. But I don't there is an issue because it has been always like this..

@dragnoir
Copy link
Contributor

A solution that fix the issue and make the UI better is preferable :)

@Krishna2323
Copy link
Contributor

Going off the original issue topic isn't a solution😄

@dragnoir
Copy link
Contributor

@Krishna2323 how can you

adding zIndex:-1 to the icon

if the icon is from a native module where you don't have access to edit it!!!

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 26, 2024

I tested on all platforms and it works just fine, we already have access to the icon container where we apply the absolute positioning styles and pointerEvents none. Let C+ decide first...

@dragnoir
Copy link
Contributor

@Krishna2323 from the doc of the component styling https://github.com/lawnstarter/react-native-picker-select#styling
You can't apply the style you mentioned on all the platforms. Your solution will not work on all devices.

@Krishna2323
Copy link
Contributor

@Krishna2323
Copy link
Contributor

If it doesn't work on android, this will work.
https://stackoverflow.com/a/60125566
But I don't think this issue is on android native.

Pls wait on for C+.

@dragnoir
Copy link
Contributor

Yes your ptoposal does not work on Android even with the workaround you mentioned.

The QA lmentioned above that this issue is within Android: Native also.

@Krishna2323
Copy link
Contributor

Krishna2323 commented Jan 26, 2024

I can't reproduce on android & ios native. I think, we already solved the android issue with the solution prop provided by the picker select repo.

Pls wait on for C+ decision instead of trying to prove my solution wrong.

@Krishna2323
Copy link
Contributor

@jules @thienlnam, can we make this external now?

@aimane-chnaif
Copy link
Contributor

aimane-chnaif commented Jan 26, 2024

We had experienced this in the past while working on dark theme for picker component.
PR: #14984, especially this change
But seems like happening again.

@aimane-chnaif
Copy link
Contributor

If making external, happy to take this as C+ as I already have context

@melvin-bot melvin-bot bot added the Overdue label Jan 29, 2024
Copy link

melvin-bot bot commented Jan 30, 2024

@Julesssss Huh... This is 4 days overdue. Who can take care of this?

@neonbhai
Copy link
Contributor

neonbhai commented Feb 4, 2024

This is a visually transparent element, applying bg color again is unnecessary. This also helps us solve the problem without touching native platform styles. Let C+ decide here, thanks!

@melvin-bot melvin-bot bot added the Overdue label Feb 4, 2024
@Krishna2323
Copy link
Contributor

backgroundColor isn't unnecessary.

backgroundColor={theme.signInPage}

@neonbhai
Copy link
Contributor

neonbhai commented Feb 4, 2024

Hey, @Krishna2323! The bg color is applied on a visually transparent area, On the UI, this is without bounding boxes, and the picker always mirrors the color of the page. Applying bg color again is unnecessary here.

This simplifies the picker element for App, also mirror the original source coming from our own fork of react-native-picker-select here. We render the touchable inputs over the icon, and visual elements.

Since the picker was not designed in app and comes from a fork, proposal here points to the minimal changes to required to fix this.

I appreciate your concerns but let C+ decide here, thank you!

@Krishna2323
Copy link
Contributor

@neonbhai, even if we don't need bg here (which I'm sure that is needed https://github.com/Expensify/App/pull/14984/files#diff-8ab8ba45ab927f1430b8d1209d187e003a9cdfe476f809e4483ef64cdba253ebR601), I would still say what matters here is the solution, which is to move the icon container behind the input, by putting zIndex to -1 or 1. Your proposal came after mine which uses the same general approach as mine, bg is totally optional thing if that needs to be changed.

Lets wait for @aimane-chnaif's decision.

@aimane-chnaif
Copy link
Contributor

Thanks for the discussions everyone. I will update shortly

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 5, 2024
Copy link

melvin-bot bot commented Feb 8, 2024

@Julesssss, @aimane-chnaif Whoops! This issue is 2 days overdue. Let's get this updated quick!

@melvin-bot melvin-bot bot added Overdue and removed Overdue labels Feb 8, 2024
@aimane-chnaif
Copy link
Contributor

None of proposed solutions work on Windows.
Awaiting proposals...

@melvin-bot melvin-bot bot removed the Overdue label Feb 12, 2024
@dragnoir
Copy link
Contributor

@aimane-chnaif do you want a solution to fix ?

@aimane-chnaif
Copy link
Contributor

I don't think we should deprecate Picker and change to push-to-page.
@dragnoir this is still being used in signin language picker.

@Krishna2323
Copy link
Contributor

@aimane-chnaif, i think it work on windows as well, can you pls recheck 🙏, we also need to se background for options

bandicam.2024-02-12.15-54-53-962.mp4

@aimane-chnaif
Copy link
Contributor

@Krishna2323 still your solution is just to add zIndex: -1, right?

@Krishna2323
Copy link
Contributor

Yes

@aimane-chnaif
Copy link
Contributor

@Krishna2323 I retested but that still causes regression.
Arrow icon disappears.

Untitled

@Krishna2323
Copy link
Contributor

@aimane-chnaif, you need to set the background color of the picker to transparent, as mentioned in my proposal

@aimane-chnaif
Copy link
Contributor

@Krishna2323 still your solution is just to add zIndex: -1, right?

You said Yes so I am confused.

Please share testing branch

@Krishna2323
Copy link
Contributor

I meant my solution also includes zIndex: -1 😅, sharing a test branch...

@dragnoir
Copy link
Contributor

@aimane-chnaif pls check #34849

The team and the contributors are on the path to replace the <Picker> with the <MenuItemWithTopDescription> due to the bugs and issues related to <Picker>

On of the update is here:
c4cdb9b#diff-18437544a71e140db50db4634f86560cda6a7f0af85c5d1ae4b4d9a2da1c0334L155

image

image

This was about the Unit component and when I tested this issue the app was using the Picker.

I think it's better to use a better component and make the UI consistent better than having the Pciker with all the bugs and seeking a workaround to edit it. What do you think?

You mentioned we already use the Picker in oither places! I think we are going on a path to replace all of those with <MenuItemWithTopDescription>

@aimane-chnaif
Copy link
Contributor

@dragnoir what about signin language picker?

@dragnoir
Copy link
Contributor

@aimane-chnaif this one ?

image

Ther'is no issue there with the arrow icon, it's clickable without issue.

@aimane-chnaif
Copy link
Contributor

@aimane-chnaif this one ?

image

Ther'is no issue there with the arrow icon, it's clickable without issue.

Interesting. What's the difference between small picker and larger picker?

@dragnoir
Copy link
Contributor

dragnoir commented Feb 13, 2024

@aimane-chnaif all Picker are now working perfectly. Seems something with the last cleans on the patches.

20240213_121551.mp4

I think we can close this issue if it's just relevant to the click issue. But it's better to use the new component <MenuItemWithTopDescription> to make the app consistent between all screens and the login page language dropdown is working perfectly.

@aimane-chnaif
Copy link
Contributor

Indeed fixed in #36117 (merged today)

@Julesssss I think we can close this as fixed unless we're going to refactor to push-to-page

@Julesssss
Copy link
Contributor

Thanks all

I think we can close this as fixed unless we're going to refactor to push-to-page

That's going to need to be a separate discussion I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

No branches or pull requests

8 participants