-
Notifications
You must be signed in to change notification settings - Fork 3.5k
[NO QA][Odometer] Create NewDot Odometer expense flow #78919
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
Conversation
| // eslint-disable-next-line no-restricted-syntax -- `backTo` usages in this file are legacy. Do not add new `backTo` params to screens. See contributingGuides/NAVIGATION.md | ||
| backTo: Routes; |
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.
There are already a huge amount of changes for this feature. We will replace backTo as the next task, here is the issue.
This comment was marked as outdated.
This comment was marked as outdated.
|
@DylanDylann @cristipaval I have fixed expense editing, and the UI issue. I asked @francoisl to review the final few commits I made this morning to address Dylan's feedback. |
Julesssss
left a comment
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.
Approving commits prior to mine ✅
And vouching for my changes. The feature is not available to users, and this is the first of many PRs to be implemented.
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeiOS: HybridAppiOS: mWeb SafariMacOS: Chrome / Safari |
|
✋ 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/Julesssss in version: 9.2.97-0 🚀
|
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.2.99-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 9.2.99-8 🚀
|
|
@jakubkalinski0 Hi, I see that you use ref value here instead of their corresponding state:
Is it possible that we use the state instead? Im asking because I'm resolving conflicts of my PR that's having changes to |
|
@mkzie2 I am not sure if you are referring to the correct code here. Those refs I use to determine whether I have to initialize the local state. If you are introducing changes to App/src/pages/iou/request/step/IOURequestStepDistanceOdometer.tsx Lines 565 to 571 in 044d6e4
Am I right? Nevertheless if you are removing the ref approach then I guess I would have no other choice but to adjust the code here. Although it will cause more rerenders since I used the refs to avoid using them as useEffect dependencies, but if the ref approach is removed the I have no other choice I guess |
|
@jakubkalinski0 We don't use refs inside If we can't replace that line then we can leave it as is. |
|
@mkzie2 I don't think I understand what you mean. How is that possible that this line which is completely unrelated to And what exactly do you mean by that:
With this new implementation of App/src/pages/iou/request/step/IOURequestStepDistanceOdometer.tsx Lines 565 to 571 in 044d6e4
|
|
@jakubkalinski0 You can take a look at my PR for more details. The changes are:
|
|
@mkzie2 Oh now i get you. Yeah, if you are migrating those refs to states anyway then in that case I see no reason why we shouldn't be able to get rid of those refs |
|
@mkzie2 Do you need my assistance with migrating those refs to states? |
|
@jakubkalinski0 Sorry I missed your comms, and thanks for the offer! I think I’m all set for now, but I’ll definitely let you know if I hit any blockers. |
This is a clone @jakubkalinski0's original PR here. Comments on original PR have been addressed there.
The Odometer tab is disabled with these changes and won't appear to users. It will be enabled in later Odometer PRs. It is necessary that we merge to main early and often to prevent conflicts, especially as the GPS tab is being worked on simultaneously.
Here are some images from testing:
Original PR description:
Explanation of Change
This PR adds an Odometer option to the distance request flow allowing user to add odometer readings at the beginning and end of the trip. This explanation and implementation is mapped according to docs (only Phase 1; selected items from 1.A–1.J). Some code fragments are added in the explanation below for clarity. This PR introduces end-to-end handling of this feature allowing to create an actual
distance-odometerexpense. This PR only allows to add odometer readings without capturing and merging images or the 'Save for later' functionality.distance-odometertab and odometer step route.TabSelectoruses the meter icon, andDistanceRequestStartPagerenders the odometer screen:end > startbefore navigation:DiscardChangesConfirmationto block accidental loss of unsaved odometer inputs when leaving the screen:Fixed Issues
$ #77191
PROPOSAL: N/A
Tests
Odometer Distance Tests
Global Create -> Track distance -> Odometer tab
Missing fields validation and End < Start validation
Correct input 100 -> 160
Back/unsaved changes - Discard Changes modal
Tab switching state
Edit on confirmation (test errors, increase distance, test leave without saving)
Existing report (non-global create)
Skip-confirmation path
Default expense policy
Offline tests
Disconnect internet from your PC or force offline through troubleshoot
Everything else should work same as described in Tests
QA Steps
Verified internally. The feature is disabled currently.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari