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.

Sign up for GitHub

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

Jump to bottom

Add IOU badge to report row and header #2095

Merged
merged 6 commits into from
Mar 29, 2021
Merged

Add IOU badge to report row and header #2095

merged 6 commits into from
Mar 29, 2021

Conversation

marcaaron
Copy link
Contributor

@marcaaron marcaaron commented Mar 26, 2021 •
edited
Loading

Details

Adds IOU badge.

Notes:

  • We still aren't yet handling the Pusher event so once the IOU is paid things do not update in realtime. This can probably be fixed in a later PR.
  • The report header text wraps weirdly now that there is an extra element. I forced it to use ellipsis instead since I think it looks better.

Fixed Issues

Fixes https://github.com/Expensify/Expensify/issues/158424

Tests

  1. Create a new IOU transaction via
CreateIOUTransaction
authToken: <auth token>
debtorEmail: <email of user you want to request payment from>
currency: USD
comment: 'test'
amount: 299
  1. Open E.cash as the user (same authToken that you just used)
  2. Verify that there is now a badge to in the report row and header
  3. Run the next command to mark it as paid
PayIOU
authToken: <auth token of the payee> // must sign in as the "debtor"
reportID: <IOU Report ID> 
paymentMethodType: 'Venmo'
  1. Refresh the page
  2. Verify that the badges are no longer displayed

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-03-26 at 7 57 47 AM

Mobile Web

Screen Shot 2021-03-26 at 8 09 37 AM

Screen Shot 2021-03-26 at 8 09 17 AM

Desktop

Screen Shot 2021-03-26 at 8 11 58 AM

iOS

Screen Shot 2021-03-26 at 8 01 16 AM

Screen Shot 2021-03-26 at 8 01 06 AM

Android

Screen Shot 2021-03-26 at 8 07 59 AM

Screen Shot 2021-03-26 at 8 07 50 AM

@marcaaron marcaaron self-assigned this Mar 26, 2021
@marcaaron marcaaron marked this pull request as ready for review March 26, 2021 00:33
@marcaaron marcaaron requested a review from a team as a code owner March 26, 2021 00:33
@botify botify requested review from ctkochan22 and removed request for a team March 26, 2021 00:33
shawnborton
shawnborton reviewed Mar 26, 2021
View reviewed changes
src/styles/styles.js Outdated
@@ -228,23 +228,18 @@ const styles = {
pill: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be worth renaming this to badge

shawnborton
shawnborton reviewed Mar 26, 2021
View reviewed changes
src/styles/styles.js Outdated
@@ -228,23 +228,18 @@ const styles = {
pill: {
borderRadius: 14,
backgroundColor: themeColors.pillBG,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll also have a gray badge that will be the default case, so we might want to think about how to manage the styles here. Maybe we have badge as well as badge.success or something? Similar to how we handle default buttons versus success buttons?

Or the default badge is green, and then we can also have badge.neutral or something?

Julesssss
Julesssss reviewed Mar 26, 2021
View reviewed changes
src/components/IOUBadge.js
style={styles.pillText}
numberOfLines={1}
>
{`$${Num.number_format(props.iouReport.total / 100, 2)}`}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NAB, but this looks like something we'll be reusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specifically, the total > dollar format

Julesssss
Julesssss reviewed Mar 26, 2021
View reviewed changes
src/styles/styles.js Outdated Show resolved Hide resolved
Julesssss
Julesssss previously approved these changes Mar 26, 2021
View reviewed changes
Copy link
Contributor

@Julesssss Julesssss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, just the conflict to resolve

Julesssss
Julesssss previously approved these changes Mar 26, 2021
View reviewed changes
@marcaaron
Copy link
Contributor Author

Updated screens lmk if this is feeling right @shawnborton !

@shawnborton
Copy link
Contributor

Hmm it doesn't feel quite right anymore, but previously it was looking right. Ideally the pill ends up being only 20px tall, the same exact height as the pin icon next to it.

@Julesssss
Copy link
Contributor

Hmm it doesn't feel quite right anymore, but previously it was looking right. Ideally the pill ends up being only 20px tall, the same exact height as the pin icon next to it.

@shawnborton whoops, that's my fault. I mentioned here that the size didn't match the Figma design. Was there a newer design somewhere?

@shawnborton
Copy link
Contributor

Ah yes, no worries there. I think there have been a couple of different sizes floating around but most recently we landed on having the row/header badges be the same height as the pin icon.

@marcaaron
Copy link
Contributor Author

No worries. Here's the updated screenshot for web (not gonna re-do them for all platforms since the change is pretty small)

Screen Shot 2021-03-29 at 6 53 56 AM

@shawnborton
Copy link
Contributor

Nice, thanks for the update. Going to merge since Jules approved.

@shawnborton shawnborton merged commit 39fe2c6 into master Mar 29, 2021
@shawnborton shawnborton deleted the marcaaron-addPill branch March 29, 2021 18:09
@OSBotify OSBotify mentioned this pull request Mar 29, 2021
Closed
85 tasks
@Luke9389
Copy link
Contributor

Hey @marcaaron, I just resolved a merge conflict that involves this PR, and want to be sure that I didn't break the functionality here. When I test this locally, should I be creating this job via /scripts/bwm.sh? If so should I just pull the auth token from the session key in local storage?

CreateIOUTransaction
authToken: <auth token>
debtorEmail: <email of user you want to request payment from>
currency: USD
comment: 'test'
amount: 299

@Luke9389 Luke9389 mentioned this pull request Mar 30, 2021
Merged
5 tasks
@marcaaron
Copy link
Contributor Author

That is an Auth command not a bedrock job.

@Luke9389
Copy link
Contributor

OK cool! Thanks for steering me in the right direction.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Julesssss Julesssss Julesssss approved these changes

@shawnborton shawnborton shawnborton approved these changes

Assignees

@marcaaron marcaaron

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@marcaaron @shawnborton @Julesssss @Luke9389