-
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
Show console debug logs via test tools menu #40656
Conversation
@eVoloshchak 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] |
Let us know when you're ready for design review 😄 |
@dubielzyk-expensify Design review can be done now. |
Doesn't look like there's anything new other than a shortcut from Test Prefs so this looks good to me. Can you confirm that's the case? We haven't tweaked the debug console screen at all? cc @Expensify/design for visibility |
@shawnborton It's the same on staging, and unrelated to these changes since we haven't changed the UI here. I think we can create an issue to fix it? |
I also checked after pulling main, it's still the same. |
Okay, cc @luacmartins - another spot where the footer button is broken |
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'll approve from the design change as this seems good to us 😄 The footer button sounds like it'll be fixed in a separate issue
@@ -91,7 +91,13 @@ function BaseShareLogList({onAttachLogToReport}: BaseShareLogListProps) { | |||
<> | |||
<HeaderWithBackButton | |||
title={translate('initialSettingsPage.debugConsole.shareLog')} | |||
onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_CONSOLE)} | |||
onBackButtonPress={() => { |
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.
This logic doesn't work correctly in some cases
24-04-25-17-10-20.mp4
But I was wondering if we need the isViaTestToolsModal
prop at all. Is its only purpose is to conditionally navigate to a different page onBackButtonPress?
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.
@ShridharGoel, friendly bump on this 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.
Yes, it is used to navigate based on the source via which the debug console was opened.
@dubielzyk-expensify, this modal doesn't look right to me, it ends up being "inside" of the test tools menu 24-04-25-17-10-45.mp4Could you please confirm what is the expected result here? A new modal that is the exact size of the test tools menu modal? A full-screen modal? Or just navigate to a "Debug console" page like it happens currently? 24-04-25-17-18-07.mp4 |
Good point. Yeah I think just navigating to the Debug Console is the right move here cause you're right that it feels weird to show the console within a modal |
@ShridharGoel, could you implement the changes described in #40656 (comment)? |
@eVoloshchak Yes, I'm working on the changes. |
text={translate('initialSettingsPage.debugConsole.viewConsole')} | ||
onPress={() => { | ||
toggleTestToolsModal(); | ||
Navigation.navigate(ROUTES.SETTINGS_CONSOLE.getRoute(Navigation.getActiveRoute())); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
But it will not always be ROUTES.HOME
I think. What if this is opened via some chat?
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.
That is true. I assumed you can't open the debug menu web (can you?), but even if you can't, let's keep Navigation.getActiveRoute()
since it's more universal. Thanks!
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.
It can't be opened on web, but on mobile it can be opened via any page.
@@ -44,6 +48,8 @@ function ConsolePage({capturedLogs, shouldStoreLogs}: ConsolePageProps) { | |||
const {translate} = useLocalize(); | |||
const styles = useThemeStyles(); | |||
|
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.
Bug:
Screen.Recording.2024-05-08.at.15.27.28.mov |
@eVoloshchak This seems to be production behaviour, can you check by using the troubleshoot page and going to debug console and coming back? The page slides from right to left. |
Reviewer Checklist
Screenshots/VideosAndroid: Native24-05-08-18-04-13.mp4Android: mWeb ChromeScreen.Recording.2024-05-08.at.18.02.29.moviOS: NativeScreen.Recording.2024-05-08.at.18.03.19.moviOS: mWeb SafariScreen.Recording.2024-05-08.at.18.08.58.movMacOS: Chrome / SafariScreen.Recording.2024-05-08.at.17.59.20.movMacOS: DesktopScreen.Recording.2024-05-08.at.18.01.04.mov |
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!
Thanks so much for the hard work, both of you! I'm putting this on HOLD because we're in a merge freeze till Monday I believe - see https://expensify.slack.com/archives/C01GTK53T8Q/p1715105636003919 |
Alright gents, this PR is off hold! @ShridharGoel can you please pull main & retest? And @eVoloshchak can you please also retest once main is pulled in? |
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.
Looks good to me, @eVoloshchak can you please review today? 🙏
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.
Tests well!
Thanks y'all! Seems we're still on the merge freeze |
@Beamanator Can we merge this? |
Oof thanks for the ping, let's merge main again and test, since the last commit was over a week ago |
Thanks! @eVoloshchak can you please test again to make sure we're good to merge? 🙏 |
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.
Looks good and tests well
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/Beamanator in version: 1.4.77-11 🚀
|
🚀 Deployed to staging by https://github.com/Beamanator in version: 1.4.77-11 🚀
|
🚀 Deployed to production by https://github.com/mountiny in version: 1.4.78-5 🚀
|
Details
Show console debug logs via test tools menu.
Fixed Issues
$ #40208
PROPOSAL: #40208 (comment)
Tests
QA Steps
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
Screen.Recording.2024-04-22.at.8.51.40.PM.mov
Android: mWeb Chrome
Screenrecording_20240422_215418.mp4
iOS: Native
Screen.Recording.2024-04-22.at.8.46.39.PM.mov
iOS: mWeb Safari
Screen.Recording.2024-04-22.at.8.58.00.PM.mov
MacOS: Chrome / Safari
MacOS: Desktop