Skip to content
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

Autofocus magic code input #15101

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Conversation

francoisl
Copy link
Contributor

@francoisl francoisl commented Feb 13, 2023

Details

When getting to the magic code page after entering a primary login, automatically focus the Magic Code input.

cc @narefyev91 @mountiny

Fixed Issues

Follow-up for #15081 - #14853

Tests

  1. Using a phone number login that is on the passwordless beta, enter the phone number on the sign-in page
  2. When you get to the Magic Code validation page, make sure the input is automatically focused into
  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

Same as tests, but will be QA'ed along with #15081

  • Verify that no errors appear in the JS console

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I verified the translation was requested/reviewed in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to 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)
    • Any internal methods bound to 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)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Note: there seems to be a bug right now with GH that's preventing people from attaching .mov files, so I had to zip them below :|

Screenshots/Videos

Web

gh15101_2.zip

Mobile Web - Chrome

Screen Recording 2023-02-13 at 11.41.04 AM.zip

Mobile Web - Safari

gh15101_1.zip

Desktop

Screen Recording 2023-02-13 at 11.47.58 AM.zip

iOS

Screen Recording 2023-02-13 at 11.47.13 AM.zip

Android

Screen Recording 2023-02-13 at 11.40.28 AM.zip

@francoisl francoisl added CP Staging InternalQA This pull request required internal QA labels Feb 13, 2023
@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label ⚠️ ⚠️
If you applied the CP Staging label before the PR was merged, the PR will be be immediately deployed to staging even if the open StagingDeployCash deploy checklist is locked.
However if you applied the CP Staging after the PR was merged it's possible it won't be CP'ed automatically. If you need it to be CP'ed to staging, tag a member of @Expensify/mobile-deployers to CP it manually, otherwise you can wait for it to go out with the next deploy.

@francoisl francoisl added InternalQA This pull request required internal QA and removed InternalQA This pull request required internal QA labels Feb 13, 2023
@francoisl francoisl self-assigned this Feb 13, 2023
@francoisl francoisl marked this pull request as ready for review February 13, 2023 19:50
@francoisl francoisl requested a review from a team as a code owner February 13, 2023 19:50
@melvin-bot melvin-bot bot requested review from Luke9389 and removed request for a team February 13, 2023 19:51
@MelvinBot
Copy link

@Luke9389 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]

@mountiny
Copy link
Contributor

mountiny commented Feb 13, 2023

Reviewer Checklist

  • I have verified the author checklist is complete (all boxes are checked off).
  • I verified the correct issue is linked in the ### Fixed Issues section above
  • I verified testing steps are clear and they cover the changes made in this PR
    • I verified the steps for local testing are in the Tests section
    • I verified the steps for Staging and/or Production testing are in the QA steps section
    • I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
  • I checked that screenshots or videos are included for tests on all platforms
  • I included screenshots or videos for tests on all platforms
  • I verified tests pass on all platforms & I tested again on:
    • Android / native
    • Android / Chrome
    • iOS / native
    • iOS / Safari
    • MacOS / Chrome / Safari
    • MacOS / Desktop
  • If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
  • I verified proper code patterns were followed (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I verified that this PR follows the guidelines as stated in the Review Guidelines
  • I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
  • If a new component is created I verified that:
    • A similar component doesn't exist in the codebase
    • All props are defined accurately and each prop has a /** comment above it */
    • The file is named correctly
    • The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
    • The only data being stored in the state is data necessary for rendering and nothing else
    • For Class Components, any internal methods passed to components event handlers are bound to 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)
    • Any internal methods bound to 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)
    • All JSX used for rendering exists in the render method
    • The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Due to urgency and simplicity of the PR and also because Francois included the screens in the OP, I am includeingn only two videos in web and ios web

Web
Screen.Recording.2023-02-13.at.20.15.43.mov
Mobile Web - Chrome
Mobile Web - Safari
Screen.Recording.2023-02-13.at.20.16.29.mov
Desktop
iOS
Android

Copy link
Contributor

@mountiny mountiny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good

@mountiny mountiny merged commit 19d8a0b into main Feb 13, 2023
@mountiny mountiny deleted the francois-autoFocusValidateCodeForm branch February 13, 2023 20:18
OSBotify pushed a commit that referenced this pull request Feb 13, 2023
…deForm

Autofocus magic code input

(cherry picked from commit 19d8a0b)
OSBotify added a commit that referenced this pull request Feb 13, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 750.667 ms → 787.660 ms (+36.993 ms, +4.9%)
App start runJsBundle 207.531 ms → 219.031 ms (+11.500 ms, +5.5%)
Open Search Page TTI 589.418 ms → 594.584 ms (+5.166 ms, +0.9%)
App start nativeLaunch 19.935 ms → 20.310 ms (+0.375 ms, +1.9%)
App start regularAppStart 0.016 ms → 0.017 ms (+0.001 ms, +6.2%)
Show details
Name Duration
App start TTI Baseline
Mean: 750.667 ms
Stdev: 26.993 ms (3.6%)
Runs: 693.6632460001856 697.4576069992036 717.3315099999309 719.8619720004499 720.5806660000235 728.0893060006201 728.553789999336 731.0806879997253 733.1575060002506 735.3773340005428 735.5289760008454 739.248565999791 740.9246679991484 742.987519999966 744.4724519997835 749.3816020004451 754.0478140003979 754.6120880004019 755.0826299991459 757.6719979997724 761.0896770004183 763.1759799998254 766.3314249999821 768.5004270002246 769.1379230003804 770.1359780002385 781.4267749991268 784.0035130009055 784.6171000003815 786.0605280008167 800.3584010004997 807.3878589998931

Current
Mean: 787.660 ms
Stdev: 31.127 ms (4.0%)
Runs: 737.7653190009296 748.7308569997549 751.3000649996102 753.9633729998022 756.1905790008605 759.3570470008999 762.2182459998876 762.3198300004005 763.0871079992503 763.5833100005984 763.8435699995607 764.3218660000712 769.4244919996709 781.6185720004141 782.91194899939 784.392059000209 785.5002190005034 789.4806290008128 789.7657960001379 794.7262430004776 796.6296929996461 797.5264030005783 804.2492859996855 804.4272910002619 807.6322830002755 809.5326370000839 811.5885129999369 824.3224979992956 826.103071000427 830.5853010006249 854.1723330002278 873.8465669993311
App start runJsBundle Baseline
Mean: 207.531 ms
Stdev: 18.540 ms (8.9%)
Runs: 180 181 181 185 188 191 191 193 194 194 195 196 197 200 205 206 207 210 212 212 212 214 215 216 220 224 227 229 233 239 240 254

Current
Mean: 219.031 ms
Stdev: 22.227 ms (10.1%)
Runs: 186 191 194 194 197 198 201 202 204 205 209 210 211 212 213 213 213 215 216 219 219 220 222 222 234 242 243 250 258 261 267 268
Open Search Page TTI Baseline
Mean: 589.418 ms
Stdev: 27.564 ms (4.7%)
Runs: 545.6670329999179 551.0739750005305 553.4346919991076 560.4787599984556 561.9221610017121 564.0742189995944 569.0315350014716 572.2426349986345 573.5193680003285 575.6844480000436 575.7637939993292 575.8844400011003 578.1622320003808 578.8299150001258 580.0865480005741 581.6551109999418 581.7129720002413 582.0711679998785 582.077922001481 583.6051029991359 587.6545820012689 589.6356210000813 595.9752200003713 598.6191000007093 603.3854980003089 610.2690839990973 611.1464029997587 628.9566249996424 629.81258200109 631.461020000279 633.9577229991555 642.2549649998546 660.6784270014614

Current
Mean: 594.584 ms
Stdev: 22.067 ms (3.7%)
Runs: 548.6424150001258 558.0387779995799 562.780843000859 566.6900229994208 567.9788010008633 568.2842619996518 572.7612309996039 576.9221200011671 577.5878909993917 578.7416180018336 582.5869549997151 586.6589359994978 587.2219650000334 590.3763030003756 590.5281569994986 596.6062429994345 596.7486979998648 597.519164999947 597.581991000101 600.812826000154 602.9874680005014 604.8844399992377 606.359659999609 607.777710000053 609.2207040004432 609.828531999141 610.3408199995756 623.7467460017651 624.2296549994498 625.3271890003234 627.4424240011722 630.4324949998409 633.6100260000676
App start nativeLaunch Baseline
Mean: 19.935 ms
Stdev: 1.523 ms (7.6%)
Runs: 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 21 21 21 21 22 22 22 23 24

Current
Mean: 20.310 ms
Stdev: 1.896 ms (9.3%)
Runs: 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 21 22 22 22 22 23 25 26
App start regularAppStart Baseline
Mean: 0.016 ms
Stdev: 0.001 ms (6.8%)
Runs: 0.013875000178813934 0.01440500095486641 0.01464799977838993 0.014812000095844269 0.014932999387383461 0.015096001327037811 0.015137000009417534 0.015380999073386192 0.015625 0.01574699953198433 0.015827998518943787 0.015828998759388924 0.015828998759388924 0.015869999304413795 0.01590999960899353 0.01603200100362301 0.01603200100362301 0.016234999522566795 0.01631700061261654 0.016439000144600868 0.016600999981164932 0.016642998903989792 0.016764000058174133 0.016885999590158463 0.01700800098478794 0.017090000212192535 0.01729300059378147 0.017578000202775 0.01769999973475933 0.018472999334335327 0.018636999651789665

Current
Mean: 0.017 ms
Stdev: 0.001 ms (5.7%)
Runs: 0.015217998996376991 0.015380999073386192 0.015827998518943787 0.015909001231193542 0.01590999960899353 0.016112999990582466 0.016356999054551125 0.016520000994205475 0.016520000994205475 0.01664300076663494 0.016682999208569527 0.016805000603199005 0.016805000603199005 0.017049001529812813 0.01717199943959713 0.017211999744176865 0.017292998731136322 0.017333999276161194 0.017375001683831215 0.0174150001257658 0.017619000747799873 0.01765899918973446 0.01782200112938881 0.017903000116348267 0.01798500120639801 0.01810700073838234 0.01810700073838234 0.0185139998793602 0.0185139998793602 0.0186769999563694 0.019286999478936195

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/mountiny in version: 1.2.71-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@francoisl
Copy link
Contributor Author

QAed on Android, mWeb Android, Chrome & Desktop; @vit's QA steps in https://expensify.slack.com/archives/C03TQ48KC/p1676327607824869?thread_ts=1676315168.864729&cid=C03TQ48KC included QA for this PR for iOS and mWeb iOS.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/francoisl in version: 1.2.71-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by https://github.com/AndrewGable in version: 1.3.28-2 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/AndrewGable in version: 1.3.28-5 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
InternalQA This pull request required internal QA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants