-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Fix receipt disappears when dismissing distance editor after receipt is generated #51519
Merged
Gonals
merged 13 commits into
Expensify:main
from
wildan-m:wildan/fix/48630-calculate-route-for-pending-trx-backup
Oct 31, 2024
Merged
Changes from all commits
Commits
Show all changes
13 commits
Select commit
Hold shift + click to select a range
3c91994
Calculate routes for pending transaction backup
wildan-m 5f511d2
fix incorrect param input
wildan-m a53236d
return if use mock data
wildan-m 614eb96
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m ce2e188
Refactor transaction state const
wildan-m e01442f
add ReadCommandParameters for transaction backup
wildan-m 94987b4
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m be7c27a
prettier & lint
wildan-m 5f29bd5
add comment
wildan-m 2c1fe42
Update missing transaction report fields by re-opening transaction re…
wildan-m 3d6e553
Merge branch 'main' of https://github.com/wildan-m/App into wildan/fi…
wildan-m 48356fa
change routeType to const
wildan-m 198ccb3
Resolve map not clickable when restoring transaction backup
wildan-m File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
import type {ValueOf} from 'type-fest'; | ||
import type CONST from '@src/CONST'; | ||
|
||
type TransactionStateType = ValueOf<typeof CONST.TRANSACTION.STATE>; | ||
|
||
export default TransactionStateType; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
@wildan-m⚠️ I noticed an issue which happens only when the RHP is dismissed for the first time -> the map component in the report resets / reloads. This doesn't happen if we open Distance RHP again and dismiss for the 2nd time.
issue.mov
I'm pretty sure that this will be reported as regression if we ship it this way, because since we fixed the issue in the sense that the map does show up now when dismissing the RHP, we now have that flicker map reset / reload on first time RHP dismissal.
Please look into this and see if you can figure out a way to not have the report map reset / reload when the RHP is dismissed for the first time and instead keep it the way it looks when it loads for the first time while RHP is still open if possible.
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.
@ikevin127 After further investigation, this issue related to my previous mentioned issue #48630 (comment) where
transactionBackup
is not exactly the same with unmodifiedtransaction
data.For instance, in backup, the pendingFields is not removed, this will cause incorrect result here:
App/src/components/ReportActionItem/MoneyRequestView.tsx
Lines 258 to 260 in 652d2ff
I think the only way to resolve that flicker issue and other fields flicker is to make the backup data exactly the same with unmodified transaction data when they are online, and it will require BE change.
@Gonals is it possible that when we call
GetRouteForBackup
we also get the same unmodified transaction data when they are online?The missing fields can be seen in previous comment #48630 (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.
@wildan-m Thanks for the investigation and explanation, I just wanted to point it out and ask @Gonals whether we should fix it now or in another issue.
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.
I found another issue where the generated map is not clickable due to pendingFields and pendingAction not being removed. Making
transactionBackup
identical to unmodifiedtransaction
when online is crucial to prevent flickering and an unclickable map, but I'm unsure if this is possible from the backend perspective.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.
I think this has a different root cause, right? Since it requires BE changes, I think it is a different bug
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.
@Gonals I agree that flickering is not directly related to the original issue and fixing it is not straightforward.