-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[HOLD for payment 2024-06-11] MEDIUM: [$500] Add button to console log to four finger tap menu #40208
Comments
Triggered auto assignment to @adelekennedy ( |
ProposalPlease re-state the problem that we are trying to solve in this issue.We need to add a button to view debug console logs via the four finger tap menu. What is the root cause of that problem?New feature. What changes do you think we should make in order to solve the problem?In the test tools modal (four finger tap menu), when client side logging is enabled, we can show an extra option to "View debug console". Clicking on this option would open another modal where we can display the existing Add the option after this: App/src/components/ClientSideLoggingToolMenu/BaseClientSideLoggingToolMenu.tsx Lines 64 to 70 in db9e01c
This new option will be visible only if Sample code for the new option:
Create a new ConsoleModal. We will extract the components of ConsolePage at a common place (called ConsoleComponents), and use them in both ConsoleModal and ConsolePage (so no duplication of code). I can share the test branch or detailed code which I have already tested. Debug.console.logs.modal.movIf we don't want options like save logs, share logs and command then we can skip them and simplify the UI to have just the logs. Bonus functionality (A suggestion) We can also add a new functionality of 5-finger tap. If 5-finger tap is done, we can:
|
ProposalPlease re-state the problem that we are trying to solve in this issue.
What is the root cause of that problem?What changes do you think we should make in order to solve the problem?
const onToggle = () => {
if (!shouldStoreLogs) {
Console.setShouldStoreLogs(true);
if (onEnableLogging) {
onEnableLogging();
}
+ if (isInTestToolModal) {
+ toggleTestToolsModal();
+ Navigation.navigate(ROUTES.SETTINGS_CONSOLE);
+ }
return;
}
...
};
What alternative solutions did you explore? (Optional)
onBackButtonPress={() => {
+ if (isTestToolModal) {
+ Navigation.goBack();
+ toggleTestToolsModal();
+ return;
+ }
Navigation.goBack(ROUTES.SETTINGS_TROUBLESHOOT);
}} ResultScreen.Recording.2024-04-14.at.11.10.34.mov |
ProposalUpdated with video and bonus functionality. |
What are the next steps here; who is this waiting on, and what will they do? Is this waiting on @adelekennedy ? |
Job added to Upwork: https://www.upwork.com/jobs/~01bb0c76728a550068 |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @eVoloshchak ( |
External label added for proposal reviews |
@eVoloshchak will you review the proposals above? |
@ShridharGoel's proposal looks good to me! 🎀👀🎀 C+ reviewed! |
Triggered auto assignment to @Beamanator, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@eVoloshchak @Beamanator The issue's requirement mentioned that:
Can you help confirm the expected behavior once we enable "client side logging"? Should we navigate user to "console" screen right after that? If yes, the selected proposal does not cover it. cc @quinthar as you are the reporter |
This clearly mentions that a button has to be added. Which means the console will show on clicking this new button (and the old button to enable logging would be as it is). |
hi all, what are the next steps, and the ETA? |
@Beamanator I think the issue is in your court to approve the proposal @eVoloshchak approved (considering @nkdengineer point here |
PR can be reviewed now: #40656 |
@eVoloshchak @Beamanator flagging that the PR is ready! |
Hi all, what's the next step and ETA on this? |
Next step = @ShridharGoel is implementing a few changes that were requested in the PR - here: #40656 (comment) Not sure of ETA |
PR has been updated. |
@adelekennedy This is marked as HIGH but it is missing the High Priority label. Can you check it? |
What is the next step, who is doing it,and when? |
Bumped C+ & Contrib in the PR - there was some concern about a crash on iOS & conflicts need to be resolved, hopefully quickly since this is a HIGH priority issue 🙏 |
@quinthar The last update on the PR was done a week back and it was waiting for final review. Today, some new comments have been added which I'll check. @Beamanator It was not a crash due to the new code changes. It seemed to be because of using "Shake" option on simulator. |
Upwork job price has been updated to $500 |
looks like this will be merged soon |
PR is ready, will be merged after the merge freeze is over |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.78-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2024-06-11. 🎊 For reference, here are some details about the assignees on this issue:
|
BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:
|
If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!
Problem:
No option in the the four-finger tap menu, which allows you to enable client side logging, to also access the realtime console logs.
Solution:
Please add a button on the four-finger tap menu, which allows you to enable client side logging, to also access the realtime console logs. I find when I want to enable it, I'm often deep in some specific context that I don't want to leave.
Context/Examples/Screenshots/Notes:
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: @quinthar
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1712979436331499
Add any screenshot/video evidence
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @adelekennedyThe text was updated successfully, but these errors were encountered: