-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Display Done and Paid badges in Search #43951
Conversation
Please note that this PR depends on API changes that are not yet merged/deployed. I'll let you know once that's ready for testing. |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
Have a couple of (probably) dumb questions:
|
API is deployed, this is ready for review/testing |
Similar comment as Danny, I think it would be nice to use Paid or Done for any expense that was paid via "Mark as manually paid" |
Great questions!
These should show the paid too. There was a bug in the API and PR with the fix is here. Thanks for catching this!
Done is shown when the report is just closed, either explicitly by the user or if the workspace setting has a submit and close configuration. So these don't have explicit reimbursements. |
Ah thanks so much for that explanation! |
@dannymcclain can you test again, the |
Yeah I agree and really like the one without icon 👍 |
Yeah that is expected, though I wonder if we should give that container a min-height of 28px so that it matches the height of when the container has a button present? This way we get the same size expense cards everywhere. As for the cancelled expense... I'm actually not sure how the heck you can make a cancelled expense but I seem to have a lot in my account, probably from testing through the past years. Probably not something we need to worry about, but if it's easy to add in, I would be in favor of adding it. |
Reviewer Checklist
Screenshots/Videos |
@shawnborton just to make sure I understand, are you suggesting we wrap the badge in a 28px tall container, or adjust the height of the badge itself to 28px? I'd be down for wrapping it to get consistent card heights, but I don't think I like the idea of changing the actual height of the badge. |
I was thinking to give the wrapper a min-height of 28px, and vertically centering everything. So no changes to the badge height or anything like that! |
Right on—then I'm all for that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good to me - will hold on approval until @s77rt's minor notes are addressed and any design updates that look like they're being discussed above.
I think both comments from @s77rt were addressed. As for the cancelled badge, I'd hold on doing that. AFAIK we deprecated cancelling IOUs in favor of delete so this shouldn't even be possible anymore 😂
I'm not sure that I get this suggestion. Wouldn't it look the same as what we have now? |
So when there is no button present, the top header area of an expense card is shorter than if there was a button present. So if we give that area a min-height of 28px, which is the height of the button, we'll always have the same height header area no matter if there is a button or not. |
Perfect, thanks! |
@dangrous this is ready for review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/dangrous in version: 9.0.2-0 🚀
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.0.3-7 🚀
|
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
Details
Displays the
Done
andPaid
badges for expense search results.Fixed Issues
$ #39890
Tests
Pre-requisite: an account with closed AND reimbursed reports
Done
andPaid
badges next to the row for the closed and reimbursed reportsOffline tests
N/A
QA Steps
Same as tests
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps./** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
![Screenshot 2024-06-18 at 1 26 41 PM](https://github.com/Expensify/App/assets/22219519/754aa84a-b2a8-4263-8ed7-1ff998e25be6) ![Screenshot 2024-06-18 at 1 26 48 PM](https://github.com/Expensify/App/assets/22219519/8188fcb3-6b00-4b23-9b59-0d4eb89800bb) ![Screenshot 2024-06-18 at 1 27 01 PM](https://github.com/Expensify/App/assets/22219519/cd5a97f4-e969-4d17-a68c-e21252ee1772) ![Screenshot 2024-06-18 at 1 27 12 PM](https://github.com/Expensify/App/assets/22219519/013f3c0c-a3a3-433b-b269-c84648c12b8c) ![Screenshot 2024-06-18 at 1 37 29 PM](https://github.com/Expensify/App/assets/22219519/41bc3b8d-c531-4d2d-b6af-b5d75855c216) ![Screenshot 2024-06-18 at 1 37 58 PM](https://github.com/Expensify/App/assets/22219519/f76140b4-7ec1-4985-8835-a86ed80581e4)MacOS: Desktop