-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
[No QA] Create trip contextual filter #47111
Conversation
@rayane-djouah Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
@rayane-djouah this PR is based on #47103, so let's review/merge that one first. |
Can you adjust the size of the trips illustration to be 191x170? I think it looks a little small right now |
Reviewer Checklist
Screenshots/VideosNavigation test recordings:Android: NativeScreen.Recording.2024-08-10.at.4.34.24.PM.movAndroid: mWeb ChromeScreen.Recording.2024-08-10.at.4.33.33.PM.moviOS: NativeSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-10.at.16.32.49.mp4iOS: mWeb SafariSimulator.Screen.Recording.-.iPhone.15.Pro.Max.-.2024-08-10.at.16.32.09.mp4MacOS: Chrome / SafariScreen.Recording.2024-08-10.at.4.30.24.PM.movMacOS: DesktopScreen.Recording.2024-08-10.at.4.31.17.PM.mov
|
Related to those screenshots above, just making sure you saw this comment @luacmartins |
Yea, I'm testing those changes right now. |
nvm, my plane wifi is not playing well with the simulators. I'll push my changes and test this once I get home. |
Ok, this should be good for another review |
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 and tests well
@puneetlath Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
Out of curiosity what's the difference between the transaction and expense search data types? |
@puneetlath we're deprecating transaction and report data types in favor of expense. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
Note: This PR probably hasn't introduced the recession. The regression has been introduced at an earlier point, and the CI system will report any PR that follows this faulty PR as a regression. |
🚀 Deployed to staging by https://github.com/puneetlath in version: 9.0.20-0 🚀
|
FYI I believe this was deployed to prod yesterday, from this checklist - #47356 |
Details
Introduced the backend changes for Trip search type. Also adds an empty view for the trip search.
FYI the trip page is inaccessible to users right now and I had to hardcode values to see the empty view.
Fixed Issues
$ #46584
Tests
trip
Book travel
RHPOffline tests
N/A
QA Steps
N/A
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.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop