-
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
[Debug Mode] Add transaction and violation data to debug mode #50745
base: main
Are you sure you want to change the base?
[Debug Mode] Add transaction and violation data to debug mode #50745
Conversation
Todo:
|
I didn't have much time as I wanted to work on this today but here goes my update! Today's update:
Todo:
|
…and-violation-data-to-debug-mode # Conflicts: # src/libs/DebugUtils.ts
Today's update:
Todo:
|
Today's update:
Todo:
|
…and-violation-data-to-debug-mode
Today's update:
Todo:
|
…and-violation-data-to-debug-mode
…bug transaction violations tab
…and-violation-data-to-debug-mode # Conflicts: # src/libs/DebugUtils.ts
Today's update:
Todo:
|
…and-violation-data-to-debug-mode
… transaction violations form types
…and-violation-data-to-debug-mode
Today's update:
Todo:
|
…and-violation-data-to-debug-mode
Yesterday I resolved all the typescript issues present in the PR |
@pac-guerreiro Many thanks. I will try to review again on this weekend |
@pac-guerreiro On the tag picker, let's remove the save button. I think we should update the value and navigate go back right after the user click on the new value. Like we did on category page Screen.Recording.2024-11-11.at.14.19.02.mov |
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-11-11.at.14.55.32.movAndroid: mWeb ChromeScreen.Recording.2024-11-11.at.14.44.08.moviOS: NativeScreen.Recording.2024-11-11.at.15.10.06.moviOS: mWeb SafariScreen.Recording.2024-11-11.at.14.42.07.movMacOS: Chrome / SafariScreen.Recording.2024-11-11.at.14.33.13.movMacOS: DesktopScreen.Recording.2024-11-11.at.14.39.51.mov |
@pac-guerreiro One left comment: #50745 (comment) |
3e8499a
to
ff0aa9b
Compare
…and-violation-data-to-debug-mode
…and-violation-data-to-debug-mode
@DylanDylann @fabioh8010 Thanks for your feedback! I addressed all your suggestions, let me know if there's anything else that needs fixing/refactoring 😄 |
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.
Look good to me 💯
…and-violation-data-to-debug-mode # Conflicts: # src/components/TagPicker/index.tsx # src/libs/DebugUtils.ts # src/pages/Debug/Report/DebugReportPage.tsx # src/types/onyx/Report.ts
@DylanDylann thanks! 🙏 I just resolved the conflicts 😄 |
@@ -31,11 +31,13 @@ type TagPickerProps = { | |||
/** Should show the selected option that is disabled? */ | |||
shouldShowDisabledAndSelectedOption?: boolean; | |||
|
|||
shouldOrderListByTagName?: boolean; |
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.
Please add prop description
src/pages/Debug/DebugTagPicker.tsx
Outdated
type DebugTagPickerProps = { | ||
policyID: string; | ||
tagName?: string; | ||
onSubmit: (item: ListItem) => void; | ||
}; |
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.
Please add descriptions for each prop
type DebugTransactionViolationsProps = { | ||
transactionID: string; | ||
}; |
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.
Please add descriptions for each prop
src/pages/Debug/ConstantPicker.tsx
Outdated
type ConstantPickerProps = { | ||
formType: string; | ||
fieldName: string; | ||
fieldValue?: string; | ||
onSubmit: (item: ListItem) => void; | ||
}; |
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.
Please add descriptions for each prop
Looks like there are lint failures. Also, this PR is huge. I would love to see in the future, that if we find the PR is growing large like this, to separate it into multiple smaller PRs. One GH issue does not have to mean one PR. It's always a good think to break a big problem into smaller chunks. For example, I think this PR could have been broken down into smaller chunks by:
|
src/types/onyx/Report.ts
Outdated
@@ -37,11 +37,14 @@ type PendingChatMember = { | |||
|
|||
/** Report participant properties */ | |||
type Participant = OnyxCommon.OnyxValueWithOfflineFeedback<{ | |||
/** Whether the participant is hidden */ | |||
hidden?: boolean; |
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.
Why are we adding all this to the report type? We recently worked in this issue to clean up the Report type to only store data that is sent by the server. #51867
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 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.
Ah gotcha. Yeah "hidden" is being removed from the server as we speak. What about the other ones. Do we need those?
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 just removed all the unused properties as agreed in #51867 😄
Thanks for the feedback @puneetlath 😄
…and-violation-data-to-debug-mode
…and-violation-data-to-debug-mode
All feedback addressed and conflicts resolved! Thanks for your feedback 😄 |
Details
Fixed Issues
$#50335
PROPOSAL:
Tests
View transaction
in details tabdetails
,json
andviolations
tabsdetails
tab is saved correctlyviolations
tab, try creating and modifying a violation and check that the UI shows your changes in real timeOffline tests
Same as tests
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.Screenshots/Videos
Android: Native
Android.-.Native.mp4
Android: mWeb Chrome
Android.-.Chrome.mp4
iOS: Native
iOS.-.Native.mp4
iOS: mWeb Safari
iOS.-.Safari.mp4
MacOS: Chrome / Safari
MacOS.-.Chrome.mp4
MacOS: Desktop
MacOS.-.Native.mp4