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

[$250] [Workspace Feeds] There's no checkmark next to the current settlement account being used. #48974

Open
6 tasks done
trjExpensify opened this issue Sep 11, 2024 · 33 comments
Assignees
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2

Comments

@trjExpensify
Copy link
Contributor

trjExpensify commented Sep 11, 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: v9.0.31-24
Reproducible in staging?: Y
Reproducible in production?: Y
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: @kevinksullivan
Slack conversation: https://expensify.slack.com/archives/C036QM0SLJK/p1725977031984379

Action Performed:

  1. Enable the Expensify Card
  2. Click Settings at the top of the Expensify Card page
  3. Click Settlement account

Expected Result:

The bank account being used should be highlighted with a checkmark

image (18)

Actual Result:

There's no checkmark against the bank account being used for settlements.

image (19)

Note: We also want to make sure we refactor the Wallet > Transfer Balance screen to use this same select list styling, as right now it has radio buttons, so let's make it consistent:

CleanShot 2024-09-10 at 16 25 20@2x

Workaround:

N/A, visual design bug.

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

See above in-line.

View all open jobs on GitHub

Issue OwnerCurrent Issue Owner: @
Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836004132424001151
  • Upwork Job ID: 1836004132424001151
  • Last Price Increase: 2024-09-17
@trjExpensify trjExpensify added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 11, 2024
Copy link

melvin-bot bot commented Sep 11, 2024

Current assignee @trjExpensify is eligible for the Bug assigner, not assigning anyone new.

@trjExpensify
Copy link
Contributor Author

Someone on the Workspace Feeds team will take this, looking for who's available.

@koko57
Copy link
Contributor

koko57 commented Sep 11, 2024

I can work on this issue

@trjExpensify
Copy link
Contributor Author

Thanks!

@koko57
Copy link
Contributor

koko57 commented Sep 11, 2024

With the non-checked bank account problem - same root cause as in #48943 (comment) - we don't have bankAccountID in the accountData object - after OpenPaymentsPage we have this data so the bank account is properly checked.
We have methodID instead, so I will check for both bankAccountID and methodID.

@trjExpensify just to make sure: we're talking of changing the Wallet > Transfer Balance page to use the same design as Settlement Account page (not the way around?) - list items instead of radio buttons?

@DylanDylann
Copy link
Contributor

@trjExpensify just to make sure: we're talking of changing the Wallet > Transfer Balance page to use the same design as Settlement Account page (not the way around?) - list items instead of radio buttons?

I see we have 2 tasks here:

  • Always display highlight checkmark on the selected option
  • we could update to align PaymentMethodList Component and use it in all places for consistency. @koko57 Wdyt?
Screenshot 2024-09-12 at 14 00 41

@DylanDylann
Copy link
Contributor

@allgandalf I will take this one

@allgandalf
Copy link
Contributor

HEROOOOOO @DylanDylann 🙇🦸

@allgandalf allgandalf removed their assignment Sep 12, 2024
@koko57
Copy link
Contributor

koko57 commented Sep 12, 2024

@DylanDylann yeah, I know we have 2 😅 I was just asking - do we want checkmarks or radio buttons? But I understand we're going with Select List not Radio Buttons 😄

@DylanDylann
Copy link
Contributor

But I understand we're going with Select List not Radio Buttons 😄

I have the same thoughts with you

Anyways, It shouldn't block creating PR, we can get a confirmation on the PR phase

@koko57
Copy link
Contributor

koko57 commented Sep 12, 2024

@DylanDylann yep, on it. I'm doing 1000 things at once, but I should be able to open the PR for that til EOD 😅

@DylanDylann
Copy link
Contributor

DylanDylann commented Sep 12, 2024

Ops. @koko57 I can help raise a PR early on Friday morning if you are busy (and you can help review it). Stay health!

@koko57
Copy link
Contributor

koko57 commented Sep 12, 2024

@DylanDylann thank you, no worries, I can do it 😃

@koko57
Copy link
Contributor

koko57 commented Sep 12, 2024

@DylanDylann @trjExpensify I can open the PR for fixing the checkmark, but for this design change I would rather open a separate PR - I can do it under its issue, but a separate would be better. It's not just replacing one component - we're using PaymentMethodList which is also used in another location, and here for the data we have both bank accounts and cards

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels Sep 12, 2024
@koko57
Copy link
Contributor

koko57 commented Sep 12, 2024

@trjExpensify @DylanDylann I'm opening the PR for review. If you decide that both task should be done in one PR I will work on it tomorrow

@trjExpensify
Copy link
Contributor Author

@trjExpensify just to make sure: we're talking of changing the Wallet > Transfer Balance page to use the same design as Settlement Account page (not the way around?) - list items instead of radio buttons?

Yes, that's correct that we want to update the Transfer balance page to use the same selectList design as the Settlement Account. 👍

@trjExpensify
Copy link
Contributor Author

@trjExpensify @DylanDylann I'm opening the PR for review. If you decide that both task should be done in one PR I will work on it tomorrow

By that you just mean that the update to a selectList in Wallet > Transfer Balance will come separately?

@koko57
Copy link
Contributor

koko57 commented Sep 13, 2024

@trjExpensify yes, the checkmark thing is a very small change and I don't want to block it, so that's why I opened the PR.
This design change will take a bit longer, but if you want I can add this to the PR with checkmark

@trjExpensify
Copy link
Contributor Author

Yep, sounds good! 👍

@koko57
Copy link
Contributor

koko57 commented Sep 13, 2024

Great! Thanks!

@mountiny
Copy link
Contributor

Are we keeping this open fo the transfer wallet page right? Or do we want to separate it into its own ticket?

@trjExpensify
Copy link
Contributor Author

May as well just use this one.

@mountiny
Copy link
Contributor

Asking in Slack if we can export this

@mountiny mountiny assigned allgandalf and unassigned koko57 Sep 17, 2024
@mountiny mountiny added the External Added to denote the issue can be worked on by a contributor label Sep 17, 2024
@melvin-bot melvin-bot bot changed the title [Workspace Feeds] There's no checkmark next to the current settlement account being used. [$250] [Workspace Feeds] There's no checkmark next to the current settlement account being used. Sep 17, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

Job added to Upwork: https://www.upwork.com/jobs/~021836004132424001151

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2024
Copy link

melvin-bot bot commented Sep 17, 2024

Current assignees @allgandalf and @DylanDylann are eligible for the External assigner, not assigning anyone new.

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Sep 17, 2024
@mountiny mountiny removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 17, 2024
@melvin-bot melvin-bot bot added Weekly KSv2 and removed Daily KSv2 labels Sep 17, 2024
@allgandalf
Copy link
Contributor

PR is on staging

@melvin-bot melvin-bot bot added Weekly KSv2 and removed Weekly KSv2 labels Sep 20, 2024
@allgandalf
Copy link
Contributor

Other PR also got merged, fixed the issue of Virtualised lists error across application 🪂

@mountiny
Copy link
Contributor

I propose $375 for this work as its 2 prs and the second pr put in place a fix with guidelines for others to avoid it so that is useful.

@allgandalf
Copy link
Contributor

Fine with me 👍

@trjExpensify
Copy link
Contributor Author

We're handling this one outside of the project, yeah?

@DylanDylann
Copy link
Contributor

Am I eligible for payment?

@mountiny
Copy link
Contributor

Yeah i would say $250 as the second pr was basically no qa through but it took Gandalf some time to figure it out.

@allgandalf
Copy link
Contributor

Update:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something is broken. Auto assigns a BugZero manager. External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
Status: Polish
Development

No branches or pull requests

5 participants