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

[HOLD for payment on July 8] Add icons to the IOU payment methods #3791

Closed
trjExpensify opened this issue Jun 29, 2021 · 13 comments · Fixed by #3852
Closed

[HOLD for payment on July 8] Add icons to the IOU payment methods #3791

trjExpensify opened this issue Jun 29, 2021 · 13 comments · Fixed by #3852
Assignees
Labels
Engineering 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 Jun 29, 2021

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

Upwork job: https://www.upwork.com/jobs/~01b50df711834398c8


Deliverables

  • Add icons to all of the IOU payment methods in the payment method selector within the IOUDetails modal

Reproduction Steps

  1. Click the + icon and select Request Money
  2. Enter an amount and click next
  3. Select an attendee and then click confirm
  4. Log-in to the account you just sent the request to
  5. Click the badge icon on the chat row in the LHN to access the IOUDetails modal
  6. Click the v on the confirmation button to access the payment method selector

Platform:

All Platforms

Version Number: v1.0.74-0
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos:

image

CC: @shawnborton can you post the assets for this?


View all open jobs on Upwork

@trjExpensify trjExpensify added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 29, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Jun 29, 2021
@trjExpensify trjExpensify added the Daily KSv2 label Jun 29, 2021
@jboniface jboniface removed their assignment Jun 29, 2021
@MelvinBot
Copy link

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

@jboniface
Copy link

lgtm

@shawnborton
Copy link
Contributor

I included the cash, PayPal, and Venmo icons here: PaymentIcons.zip

The wallet icon already exists in the E.cash repo.

@parasharrajat
Copy link
Member

parasharrajat commented Jun 29, 2021

Seems like a quick one. We need to pass the icon to menu items here https://github.com/Expensify/Expensify.cash/blob/b49e2221f74f04e664f10a0675c4d4fe7cc5de07/src/pages/iou/IOUDetailsModal.js#L260.

Thus we update our Button component to add support for an optional Icon. We can pass custom ContentComponent to the Button but I think it would be best to support icon natively in the button Component.

Then based on the selected payment Option we pass down the Icon as well as text to the Button.

@MonilBhavsar MonilBhavsar added the External Added to denote the issue can be worked on by a contributor label Jun 30, 2021
@MelvinBot
Copy link

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

@parasharrajat
Copy link
Member

Please let me know if I am good to submit a PR. Thanks.

@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

Thus we update our Button component to add support for an optional Icon. We can pass custom ContentComponent to the Button but I think it would be best to support icon natively in the button Component.

I'm not 100% clear on this, what Button component needs to be updated? Couldn't we just create a paymentTypeIconOptions map like we do for paymentTypeTextOptions and set the icon value where we set the text?

@parasharrajat
Copy link
Member

I added that step as I think when we select a payment mode that option should be visible on the button with its icon. If this is not required and we can skip this step.

@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

Gotcha, I think that's a good clarification so let's double-check with @trjExpensify: after selecting the payment method, should the button below include the icon or just the text (The green Pay with Expensify button)?

If we do decide to add the icon to the button, that part of your proposal looks good to me @parasharrajat, so feel free to submit a PR once we clarify that last piece.

@Jag96 Jag96 added Weekly KSv2 and removed Daily KSv2 labels Jul 1, 2021
@trjExpensify
Copy link
Contributor Author

Nah, we're not looking to add the icons to the confirmation button. 👍

@Jag96 Jag96 added the Reviewing Has a PR in review label Jul 1, 2021
@Jag96 Jag96 changed the title Add icons to the IOU payment methods [HOLD for payment on July 8] Add icons to the IOU payment methods Jul 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 1, 2021

Reopening to keep track of payment

@Jag96 Jag96 reopened this Jul 1, 2021
@Jag96
Copy link
Contributor

Jag96 commented Jul 9, 2021

Paid!

@Jag96 Jag96 closed this as completed Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants