Open ShridharGoel opened 3 weeks ago
@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
Only minor comment from me is that it feels like the button is too close to the bottom?
We had a regression related to that recently, so you might just need to pull main to fix that?
@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
@dubielzyk-expensify, this modal doesn't look right to me, it ends up being "inside" of the test tools menu
https://github.com/Expensify/App/assets/9059945/91df0a10-aafe-47c6-8185-2fe09a1ba327
Could 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?
https://github.com/Expensify/App/assets/9059945/b5732a60-138b-429d-884b-596e2c840853
Could 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?
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 https://github.com/Expensify/App/pull/40656#issuecomment-2078535642?
@eVoloshchak Yes, I'm working on the changes.
@eVoloshchak Updated.
@eVoloshchak Can you check this when possible?
@ShridharGoel, are you able to test this on iOS? For me the app crashes when you open a debug menu
https://github.com/Expensify/App/assets/9059945/3d9b7b8b-b05a-47d8-af76-3fd55eb35328
@eVoloshchak Yeah, you can use Command+D to get the test menu option.
@eVoloshchak Were you able to check it?
@ShridharGoel looks like you've got some conflicts, can you please fix? And @eVoloshchak @ShridharGoel did y'all get the iOS crash resolved?
@eVoloshchak can you confirm if the crash you noted still exists, or if you agree this has something to do with the "Shake" option in the simulator? (see this comment)
@ShridharGoel when can you get to the latest review comments & fixing the conflicts here?
can you confirm if the crash you noted still exists
@Beamanator, the crash issue still exists, verified it is also present om main, so unrelated to this PR
@Beamanator Some changes are done, need discussion on this: https://github.com/Expensify/App/pull/40656#discussion_r1591470739
@eVoloshchak Can you have a look?
Bug:
View console
button00:08
00:14
00:14
https://github.com/Expensify/App/assets/9059945/0b2c1d7b-410a-44ec-8132-c09a6525ed51
@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.
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and not onIconClick
).myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components using Avatar
have been tested & I retested again)/** comment above it */
this
properly so there are no scoping issues (i.e. for onClick={this.submit}
the method this.submit
should be bound to this
in the constructor)this
are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this);
if this.submit
is never passed to a component event handler like onClick
)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG
)Avatar
is modified, I verified that Avatar
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 the Test
steps.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?
Details
Show console debug logs via test tools menu.
Fixed Issues
$ https://github.com/Expensify/App/issues/40208 PROPOSAL: https://github.com/Expensify/App/issues/40208#issuecomment-2053741494
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
https://github.com/Expensify/App/assets/35566748/ad587266-097c-424e-be08-28da799e045dAndroid: mWeb Chrome
https://github.com/Expensify/App/assets/35566748/67b03442-8ac6-4fe3-8e7e-220775bc21f1iOS: Native
https://github.com/Expensify/App/assets/35566748/f86d2138-d1c3-4c88-9a57-f76fa201bd94iOS: mWeb Safari
https://github.com/Expensify/App/assets/35566748/4fdc54e0-99cc-4206-95c7-fef9273f3285MacOS: Chrome / Safari
MacOS: Desktop