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

[No QA] [TS migration] Migrate 'PopoverProvider' component to TypeScript #29857

Conversation

kubabutkiewicz
Copy link
Contributor

@kubabutkiewicz kubabutkiewicz commented Oct 18, 2023

Details

Fixed Issues

$ #24989

Tests

  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Click on Fab icon
  2. Verify all is good
  3. Go to some Chat
  4. Click + icon
  5. Verify all is good
  6. Open Emojis popover
  7. Scroll through it
  8. Close it with Escape key
  • 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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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
    • If we are not using the full Onyx data that we loaded, I've added the proper selector in order to ensure the component only re-renders when the data it is using changes
    • 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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.
  • If the 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.
  • I have checked off every checkbox in the PR author checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
android.mp4
Android: mWeb Chrome
iOS: Native
ios.mp4
iOS: mWeb Safari
MacOS: Chrome / Safari
web.mp4
MacOS: Desktop
desktop.mp4

@kubabutkiewicz kubabutkiewicz marked this pull request as ready for review November 3, 2023 07:11
@kubabutkiewicz kubabutkiewicz requested a review from a team as a code owner November 3, 2023 07:11
@melvin-bot melvin-bot bot requested review from cristipaval and removed request for a team November 3, 2023 07:12
Copy link

melvin-bot bot commented Nov 3, 2023

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

@cristipaval
Copy link
Contributor

cristipaval commented Nov 3, 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: mWeb Chrome
    • iOS: Native
    • iOS: mWeb 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 the left part of a conditional rendering a React component is a boolean and NOT a string, e.g. myBool && <MyComponent />.
    • 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 grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is 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 code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • 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 the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • 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.
  • If the 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.
  • I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

Screenshots/Videos

Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop

@cristipaval cristipaval merged commit 7ea89c9 into Expensify:main Nov 3, 2023
13 of 15 checks passed
@OSBotify
Copy link
Contributor

OSBotify commented Nov 3, 2023

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@github-actions github-actions bot added the DeployBlockerCash This issue or pull request should block deployment label Nov 3, 2023
Copy link
Contributor

github-actions bot commented Nov 3, 2023

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1077.828 ms → 1202.701 ms (+124.873 ms, +11.6%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1077.828 ms
Stdev: 26.689 ms (2.5%)
Runs: 1023.4963960000314 1035.4796849999111 1038.769370000111 1039.5533849999774 1041.3889440000057 1043.8013339999598 1045.2934729999397 1046.757784999907 1048.6129739999305 1048.7505749999546 1051.148109999951 1051.4533889999148 1052.2330209999345 1055.2086819999386 1057.6488069999032 1061.1492769999895 1061.203991000075 1061.572250999976 1063.673070000019 1063.8855840000324 1064.6680699998979 1066.7344100000337 1068.0319860000163 1068.964755000081 1069.87784500001 1071.1390470000915 1072.4965900001116 1072.543624999933 1074.1606310000643 1075.0299649999943 1076.5810819999315 1076.9005909999833 1077.9350660000928 1078.779700000072 1080.571995999897 1080.7674169999082 1082.8102110000327 1084.0619429999497 1086.9028380000964 1086.9259260001127 1087.456422999967 1087.988190999953 1092.8270779999439 1093.0925169999246 1093.8642820001114 1094.3118890000042 1095.4613429999445 1101.7569379999768 1104.4799709999934 1105.5634880000725 1106.806770999916 1107.4107019999065 1110.1332960000727 1110.223195000086 1111.624526000116 1114.098411000101 1129.145935999928 1131.5893149999902 1140.4500659999903 1144.4424980001058

Current
Mean: 1202.701 ms
Stdev: 37.331 ms (3.1%)
Runs: 1122.8543490003794 1124.8568379990757 1127.0366750005633 1127.1285050008446 1134.4084740001708 1139.8337730001658 1142.804333999753 1143.6547439992428 1147.98862599954 1158.8144119996578 1162.9649299997836 1176.7737140003592 1178.4988100007176 1182.873391000554 1183.6904770005494 1184.5634100008756 1185.6731829997152 1186.5445160008967 1186.962504999712 1187.4965589996427 1188.4733799993992 1192.0932439994067 1199.3328540008515 1201.154550999403 1201.2673870008439 1204.4283809997141 1205.2763480003923 1205.8467130009085 1206.1287019997835 1209.9685800001025 1210.5497990008444 1210.7087960001081 1211.4451390001923 1212.6198139991611 1215.6391390003264 1216.3740120008588 1217.4587919991463 1218.4263150002807 1219.4514579996467 1219.6232769992203 1223.8802160006016 1224.0930460002273 1224.4637400005013 1224.4777680002153 1226.4320439994335 1227.7069879993796 1228.568894000724 1231.550862999633 1237.4791800007224 1238.4180260002613 1241.866941999644 1244.221582999453 1248.3250980004668 1248.721129000187 1250.982017999515 1251.4978570006788 1257.5239279996604 1257.6976190004498 1260.736052999273 1261.7363339997828

Meaningless Changes To Duration

Show entries
Name Duration
App start runJsBundle 737.233 ms → 833.733 ms (+96.500 ms, +13.1%)
App start nativeLaunch 20.655 ms → 22.690 ms (+2.035 ms, +9.9%)
App start regularAppStart 0.017 ms → 0.015 ms (-0.001 ms, -7.9%)
Open Search Page TTI 708.311 ms → 699.663 ms (-8.648 ms, -1.2%)
Show details
Name Duration
App start runJsBundle Baseline
Mean: 737.233 ms
Stdev: 24.658 ms (3.3%)
Runs: 691 703 703 704 704 704 706 712 712 713 713 714 715 716 717 719 719 720 721 727 727 727 728 729 730 731 732 732 732 734 734 735 736 738 738 738 738 741 742 742 744 746 746 747 747 754 757 757 760 764 764 769 773 777 780 780 783 783 787 799

Current
Mean: 833.733 ms
Stdev: 29.490 ms (3.5%)
Runs: 775 778 779 781 782 785 787 788 789 800 811 812 813 817 818 818 819 820 821 825 827 828 828 829 829 829 833 833 834 834 835 836 839 840 843 843 843 845 845 846 846 849 850 850 850 851 856 857 859 860 860 862 863 863 865 881 883 889 894 899
App start nativeLaunch Baseline
Mean: 20.655 ms
Stdev: 1.504 ms (7.3%)
Runs: 18 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 23 23 24 25 25

Current
Mean: 22.690 ms
Stdev: 1.932 ms (8.5%)
Runs: 19 19 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 23 23 23 24 24 24 24 24 24 24 25 25 25 25 25 25 25 25 25 25 28 28
App start regularAppStart Baseline
Mean: 0.017 ms
Stdev: 0.001 ms (4.6%)
Runs: 0.014933000085875392 0.015055000083521008 0.01521899993531406 0.015340999932959676 0.015746999997645617 0.015787999844178557 0.0157880000770092 0.015829000156372786 0.015949999913573265 0.01595099992118776 0.01595099992118776 0.016032000072300434 0.01607199991121888 0.016112999990582466 0.016112999990582466 0.016112999990582466 0.01615400006994605 0.016235999995842576 0.016276999842375517 0.016316999914124608 0.0163569999858737 0.016398000065237284 0.016398999840021133 0.0165200000628829 0.01656099990941584 0.016600999981164932 0.016601999988779426 0.016601999988779426 0.016682999907061458 0.0166830001398921 0.016723999986425042 0.016723999986425042 0.016723999986425042 0.016724000219255686 0.016765000065788627 0.016765000065788627 0.01684599998407066 0.01688600005581975 0.016967000206932425 0.01704900013282895 0.0170889999717474 0.017089999979361892 0.017171000130474567 0.01721199997700751 0.01721199997700751 0.01729299989528954 0.017293999902904034 0.017333999974653125 0.017497000051662326 0.017537000123411417 0.017699999967589974 0.01818799995817244 0.01818799995817244 0.01827000011689961 0.01827000011689961

Current
Mean: 0.015 ms
Stdev: 0.001 ms (4.7%)
Runs: 0.013671999797224998 0.013754000887274742 0.013875998556613922 0.013915998861193657 0.013916000723838806 0.014159999787807465 0.014485999941825867 0.014567000791430473 0.014607999473810196 0.01464799977838993 0.014730000868439674 0.014810999855399132 0.014812000095844269 0.01489199884235859 0.014932999387383461 0.01493300125002861 0.014973999932408333 0.015015000477433205 0.015015000477433205 0.01505500078201294 0.015095999464392662 0.015177000313997269 0.01521800085902214 0.015259001404047012 0.015298999845981598 0.015298999845981598 0.015300000086426735 0.01533999852836132 0.015380000695586205 0.015380999073386192 0.015380999073386192 0.015420999377965927 0.015421001240611076 0.015422001481056213 0.015461999922990799 0.015461999922990799 0.015502998605370522 0.01550300046801567 0.015544001013040543 0.015706000849604607 0.015706999227404594 0.015707001090049744 0.01574699953198433 0.01574699953198433 0.015786999836564064 0.0157880000770092 0.01590999960899353 0.015990998595952988 0.01603199914097786 0.01603200100362301 0.016112999990582466 0.016276000067591667 0.016276000067591667 0.01648000068962574 0.016600999981164932 0.016683001071214676 0.016927000135183334
Open Search Page TTI Baseline
Mean: 708.311 ms
Stdev: 42.228 ms (6.0%)
Runs: 629.5026449998841 634.9933670000173 642.6944579998963 650.5377199999057 654.9338380000554 657.4906009999104 658.96822099993 661.7703459998593 665.851074999664 668.2450359999202 668.5851239999756 669.1320799998939 669.9297289997339 670.4278159998357 670.5445150001906 670.5682780002244 673.3747560000047 675.0653479998 675.4526370000094 678.4459230001085 680.7736820001155 685.7330330000259 688.003866000101 689.3929450004362 690.4333909996785 690.4968670001253 693.0944830002263 695.5967210000381 696.6210130001418 706.2475180001929 707.9346929998137 710.3632000000216 710.9827890000306 714.6132409996353 717.3882240001112 723.3552660001442 726.8239750000648 726.970215999987 728.1199149996974 730.8940030001104 731.7384850000963 732.7922780001536 734.9794509997591 736.2165529998019 741.0952550000511 741.9823409998789 742.819376999978 749.3435879996978 750.0783689999953 753.7105309995823 754.9856769996695 756.3142499998212 756.6391200004146 759.6259770002216 763.5542399999686 766.0601809998043 772.9026699997485 787.9093019999564 791.2908529997803 814.2950849998742

Current
Mean: 699.663 ms
Stdev: 32.016 ms (4.6%)
Runs: 633.4333100002259 633.6173099987209 648.1518159992993 650.204264998436 650.5915529988706 652.1809090003371 658.9312740005553 660.5486649982631 667.421916000545 667.6650799997151 667.7898360006511 670.9404300004244 673.8733319975436 676.828940000385 678.6072190012783 678.624634001404 682.6267910003662 682.8863529972732 683.3738200031221 684.1363130025566 685.4853509999812 685.720052998513 686.0128180012107 688.8254400007427 689.0222180001438 689.6699620001018 690.6184489987791 690.8244230002165 691.2992759998888 693.553630001843 695.7112229987979 695.9049890004098 696.3156739994884 698.4954429995269 703.3109949994832 703.3420000001788 710.7698569986969 710.947143997997 712.2724609989673 713.8236489985138 718.8249920010567 719.681192997843 720.0394700001925 721.6749280001968 722.6520599983633 723.238647999242 723.7235109992325 724.9200040008873 725.7858889997005 728.1808269992471 730.4496260005981 734.546590000391 736.9742440003902 743.522217001766 743.628703000024 744.9070229977369 746.7749030012637 746.8752450011671 750.3233649991453 756.7648120000958 781.598105000332

Copy link
Contributor

github-actions bot commented Nov 3, 2023

@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker.

@OSBotify
Copy link
Contributor

OSBotify commented Nov 6, 2023

🚀 Deployed to staging by https://github.com/cristipaval in version: 1.3.96-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Nov 9, 2023

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.96-15 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/puneetlath in version: 1.3.97-7 🚀

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
DeployBlockerCash This issue or pull request should block deployment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants