Open a test tools modal with 5 taps on dev #13900
Conversation
neil-marcellini
changed the title
neil-marcellini
changed the title
melvin-bot
bot
requested review from
mananjadhav and
techievivek
and removed request for
a team
|
@mananjadhav @techievivek One of you needs to 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] |
|
@techievivek do you have an Android device? Would you be willing to help me try to test on Android chrome with a physical device? I'm hoping the bug I observed is only on the simulator. |
|
🎯 @mananjadhav, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #14993. |
| @@ -72,6 +76,19 @@ class ScreenWrapper extends React.Component { | |||
| } | |||
|
|
|||
| render() { | |||
| // Open the test tools menu on 5 taps in dev only | |||
| const isDevEnvironment = this.props.environment === CONST.ENVIRONMENT.DEV; | |||
There was a problem hiding this comment.
@neil-marcellini Should this be here or still an outer layer?
I tried the taps on Login page and it didn't work? Now I am not sure if we would even need it there, but we do have controls like sign-in and language change which cannot be tested offline if the Test modal won't show up.
There was a problem hiding this comment.
That's a good point. It will be good to place it in a more outer layer.
There was a problem hiding this comment.
I added this to the ScreenWrapper since it appears to wrap almost every page. The public screens that have content other than a loading indicator, such as the SetPasswordPage and the SignInPage, do not use the screen wrapper.
I could try to use the ScreenWrapper on these pages but I would have to test how that works. Alternatively I could create another wrapper component specifically for this gesture and use it on these public pages as well in the screen wrapper.
I think it's relatively rare to test changing offline state on the public screens, so maybe I could implement that with a follow up PR sometime next week? Does that work for you guys?
There was a problem hiding this comment.
Yeah a follow up PR sounds okay to me.
This comment was marked as outdated.
This comment was marked as outdated.
|
@neil-marcellini While it worked fine for all the platforms, I am able to reproduce the glitch on the physical device too. android-test-modals-glitch.mp4 |
|
@mananjadhav Ok, thanks for testing on the physical device! I'll look into why that's failing on Android Chrome later this week. In the meantime, please let me know if you have any ideas about why it's not working there. |
|
@mananjadhav @techievivek I did some debugging on Android Chrome and I found that for whatever reason the onBackdropPress callback in the BaseModal was getting called. I think Android Chrome might not be able to keep up with the rapid taps or something like that. I ended up throttling a function to toggle the modal visibility, that way a quick press after it mounts won't close it. It's slightly hacky but I think it's fine because I don't think most people will use this on the android emulator with Chrome. You can simply turn off the wifi on the android emulator. Please review again 😄 |
|
@neil-marcellini I agree that it is fine to have a throttle because it is only for dev environment. But it still doesn't work for Android Chrome. mweb-chrome-throttle-test-modals.mov |
|
@neil-marcellini Tried the same on physical device, still the popup closes immediately. mweb-android-test-tools.mp4 |
|
Hmm that's odd @mananjadhav, it works for me. I did find that it didn't always work, such as if I did 4 or 6 taps, or if I held for too long on the last tap. I'm not really sure how to improve it here so hopefully this is good enough and we can merge this and move on. @techievivek please review. |
|
Yeah, I think for v1 this is good, and if we want to improve it, we can look into it later. |
|
Okay noted thanks @techievivek @neil-marcellini. We just need to be sure that this doesn't come up as a regression during the QA. |
|
I had this checklist completed earlier too with all the screencasts from different platforms. |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
🚀 Deployed to staging by https://github.com/techievivek in version: 1.2.76-0 🚀
|
|
I'm reverting this because it caused a regression 😞. It was mentioned in Slack here. |
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀
|
1 similar comment
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀
|
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.2.76-7 🚀
|
cc @Beamanator
Details
On dev only, you can now open a test tools modal by tapping anywhere in the app 5 times, making it easy to toggle force offline from any page.
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/246352
PROPOSAL: N/A
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.Screenshots/Videos
Web
web.mov
Mobile Web - Chrome
androidChrome.mov
Mobile Web - Safari
iOSSafari.mp4
Desktop
desktop.mov
iOS
iOS.mp4
Android
android.mov