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 'KeyboardDismissingFlatList' component to TypeScript #29989

Conversation

VickyStash
Copy link
Contributor

Details

During TS migration it turned out this component is not used anymore, so this PR removes the unused KeyboardDismissingFlatList component.

Fixed Issues

$ #25001
PROPOSAL: N/A

Tests

  • Verify that no errors appear in the JS console

Offline tests

N/A

QA Steps

  • 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.

@VickyStash VickyStash marked this pull request as ready for review October 23, 2023 07:42
@VickyStash VickyStash requested a review from a team as a code owner October 23, 2023 07:42
@melvin-bot melvin-bot bot requested review from abdulrahuman5196 and removed request for a team October 23, 2023 07:42
@melvin-bot
Copy link

melvin-bot bot commented Oct 23, 2023

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

@abdulrahuman5196
Copy link
Contributor

abdulrahuman5196 commented Oct 26, 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
Screen.Recording.2023-10-27.at.12.44.02.AM.mp4
Android: mWeb Chrome
Screen.Recording.2023-10-27.at.12.47.03.AM.mp4
iOS: Native
Screen.Recording.2023-10-27.at.12.42.57.AM.mp4
iOS: mWeb Safari
Screen.Recording.2023-10-27.at.12.46.44.AM.mp4
MacOS: Chrome / Safari
Screen.Recording.2023-10-27.at.12.39.22.AM.mp4
MacOS: Desktop
Screen.Recording.2023-10-27.at.12.48.52.AM.mp4

Copy link
Contributor

@abdulrahuman5196 abdulrahuman5196 left a comment

Choose a reason for hiding this comment

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

Changes looks good and works well. Reviewers checklist is also complete.

All yours.

🎀 👀 🎀
C+ Reviewed

@melvin-bot
Copy link

melvin-bot bot commented Oct 26, 2023

We did not find an internal engineer to review this PR, trying to assign a random engineer to #25001 as well as to this PR... Please reach out for help on Slack if no one gets assigned!

@melvin-bot melvin-bot bot requested a review from thienlnam October 26, 2023 19:20
@thienlnam thienlnam merged commit f06f1d6 into Expensify:main Oct 26, 2023
15 of 17 checks passed
@OSBotify
Copy link
Contributor

✋ 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 Oct 26, 2023
@github-actions
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
App start TTI 1248.435 ms → 1312.284 ms (+63.849 ms, +5.1%) 🔴
App start runJsBundle 869.322 ms → 919.814 ms (+50.492 ms, +5.8%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1248.435 ms
Stdev: 37.592 ms (3.0%)
Runs: 1165.0011219999287 1179.82481000002 1179.8571969999466 1180.690918999957 1185.2907120001037 1194.1481029998977 1195.2505429999437 1202.8241399999242 1209.5531250000931 1216.753861000063 1217.3761110000778 1221.9599450000096 1222.6951890001073 1222.9607859998941 1224.4166339999065 1224.9136880000588 1225.3089900000487 1230.4035459998995 1230.6926990000065 1232.3306529999245 1235.492428000085 1235.904894999927 1236.072550999932 1238.5022670000326 1240.2958770000841 1242.8087500000838 1246.4396329999436 1247.1250020000152 1248.771599000087 1250.4302610000595 1252.8003310000058 1252.829262000043 1254.1584560000338 1255.182734000031 1256.9315239998978 1257.3414129999 1257.3564150000457 1257.6655540000647 1258.72634500009 1260.662274000002 1262.2647639999632 1263.9810969999526 1265.0654200001154 1266.780368000036 1276.1028199999128 1277.5199259999208 1278.2491039999295 1279.82438799995 1282.4197279999498 1288.6172760000918 1291.9113640000578 1292.6414189999923 1293.6837639999576 1299.4404810001142 1300.0973459999077 1310.8507119999267 1317.766401000088 1320.7989759999327 1341.9184109999333

Current
Mean: 1312.284 ms
Stdev: 44.106 ms (3.4%)
Runs: 1211.5233459998854 1227.6476429998875 1241.5061800000258 1247.2128550000489 1250.9167730000336 1251.7829219999257 1260.3090190000366 1260.5135319998953 1267.818740000017 1268.8768170000985 1269.1615200000815 1269.2890319998842 1275.7574750001077 1276.126813000068 1283.5384249999188 1284.6256929999217 1284.801628000103 1287.5703970000613 1287.8470060001127 1288.4289859998971 1290.438266000012 1293.1472930000164 1294.5008940000553 1296.6091020000167 1296.9248530000914 1302.304012000095 1302.6537389999721 1303.7324270000681 1305.4874239999335 1307.5726910000667 1308.5782999999356 1314.6018739999272 1315.6862870000768 1319.5589439999312 1322.8280509999022 1327.1206420001108 1327.6813769999426 1331.5796970000956 1333.6037830000278 1338.4926320000086 1341.8044300000183 1342.4513689999003 1342.6938259999733 1345.397815000033 1346.909365999978 1348.081511999946 1349.386414000066 1353.8288869999815 1354.609293000074 1359.7613939999137 1359.8385989998933 1360.3936159999575 1367.8927980000153 1367.965804999927 1368.7294209999964 1383.6493720000144 1383.8876690000761 1395.4641140000895 1423.676017000107
App start runJsBundle Baseline
Mean: 869.322 ms
Stdev: 29.842 ms (3.4%)
Runs: 797 809 814 822 827 832 833 834 837 843 843 846 847 848 851 852 853 855 856 857 859 859 861 861 862 862 863 863 864 868 868 870 874 874 875 875 878 883 883 885 889 890 891 892 892 894 894 895 895 896 897 900 901 902 904 904 922 939 950

Current
Mean: 919.814 ms
Stdev: 32.435 ms (3.5%)
Runs: 848 852 864 868 876 881 881 881 885 885 887 890 893 897 897 898 902 903 904 904 904 906 908 909 909 910 914 916 919 919 922 923 924 925 925 929 930 931 931 932 933 934 934 942 945 948 948 958 958 959 960 963 964 967 968 970 974 976 986

Meaningless Changes To Duration

Show entries
Name Duration
Open Search Page TTI 719.057 ms → 732.202 ms (+13.146 ms, +1.8%)
App start nativeLaunch 21.877 ms → 23.086 ms (+1.209 ms, +5.5%)
App start regularAppStart 0.015 ms → 0.017 ms (+0.002 ms, +13.5%)
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 719.057 ms
Stdev: 50.486 ms (7.0%)
Runs: 643.4552820001263 655.2117929998785 656.757934999885 658.1055100001395 660.7798669999465 661.805623000022 666.1315930001438 666.989991000155 669.5808110001963 671.2554119999986 673.8777680001222 675.6051839999855 676.3446450000629 676.6798910000362 679.4405929998029 680.9060470000841 681.8529870000202 682.2991130000446 683.3159179999493 684.3940030001104 684.4890140001662 684.4931240000296 685.4021410001442 685.6983229999896 697.1750090001151 697.8585210000165 701.3480229999404 703.8683680000249 704.6881099999882 705.7777110000607 705.7784840001259 711.9213060000911 712.9985350000206 715.007487000199 715.6282560001127 717.4688719999976 721.1901050000452 722.0955409999005 723.1533210000489 723.9561769999564 734.0356850000098 734.251750000054 737.4463710000273 745.3752040001564 746.9506429999601 754.236491000047 757.5387379999738 765.7677820001263 766.4221200000029 767.4720860000234 769.0518799999263 769.7897949998733 777.8532710000873 780.5386969998945 784.7258299998939 796.6868089998607 805.3168949999381 807.8341479999945 827.4686690000817 840.2920329999179 868.6205649999902

Current
Mean: 732.202 ms
Stdev: 38.973 ms (5.3%)
Runs: 661.7918699998409 667.4713949998841 676.0657150000334 678.5592859999742 681.555338999955 683.8968509999104 687.9328209999949 690.7604169999249 691.253539999947 696.3217780000996 698.284587000031 698.3101810000371 699.5971269998699 700.9575609997846 703.1440440001898 704.8675139998086 705.0149330000859 706.9311529998668 708.0934650001582 709.1672360000666 710.01408000011 712.117838999955 714.6612140000798 715.5463870000094 716.463868000079 717.3228360000066 717.3798430000897 720.3830570001155 721.3436690000817 722.4569089999422 722.9592290001456 723.3715010001324 724.6888429999817 726.612548999954 731.1363119999878 733.1713060000911 733.2635099999607 734.4397390000522 738.7142330002971 746.5249430001713 747.4787599998526 750.0432939999737 756.5870369998738 759.0742999999784 759.9375820001587 762.3334559998475 764.2638759999536 764.4133300001267 765.0506190001033 768.4569089999422 769.1727710000705 773.3177089998499 777.1573089999147 780.6587329995818 787.1556400000118 788.1343590002507 790.7188719999976 798.7365310001187 810.0028490000404 824.4455580001231 834.6567389999982
App start nativeLaunch Baseline
Mean: 21.877 ms
Stdev: 2.890 ms (13.2%)
Runs: 18 18 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 23 23 23 23 24 24 25 25 25 25 26 27 29 29 30 30

Current
Mean: 23.086 ms
Stdev: 2.037 ms (8.8%)
Runs: 18 19 20 20 20 20 20 21 21 21 21 22 22 22 22 22 22 22 22 22 22 23 23 23 23 23 23 23 23 23 23 23 23 23 23 23 24 24 24 24 24 24 24 24 25 25 25 25 25 25 25 26 26 26 26 27 27 28
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (5.2%)
Runs: 0.013183000031858683 0.013386999955400825 0.0134680001065135 0.013671000022441149 0.013711999985389411 0.013712000101804733 0.013712000101804733 0.013712999993003905 0.013753999955952168 0.013794000027701259 0.013794000027701259 0.013916000025346875 0.013956000097095966 0.013956000097095966 0.013956999871879816 0.013998000184074044 0.014038000022992492 0.014078999869525433 0.014119999948889017 0.01424099993892014 0.014241999946534634 0.014241999946534634 0.014283000025898218 0.01432300009764731 0.014362999936565757 0.014364000177010894 0.014403999783098698 0.014405000023543835 0.014445000095292926 0.014445000095292926 0.014526000013574958 0.014567000092938542 0.014567000092938542 0.014567000092938542 0.014648000011220574 0.014649000018835068 0.014688999857753515 0.014810999855399132 0.014851999934762716 0.0148930000141263 0.014932999853044748 0.014933000085875392 0.014933999860659242 0.015014000004157424 0.015015000011771917 0.015055000083521008 0.015257999999448657 0.015339999925345182 0.015381000004708767 0.015421999851241708 0.015422000084072351 0.015461999922990799 0.015461999922990799 0.015625 0.015868999995291233 0.015910000074654818 0.015949999913573265 0.01631700014695525 0.01684599998407066

Current
Mean: 0.017 ms
Stdev: 0.001 ms (6.6%)
Runs: 0.014405000023543835 0.014484999934211373 0.014526000013574958 0.014649000018835068 0.014811000088229775 0.014933000085875392 0.014973999932408333 0.015379999997094274 0.015461999922990799 0.015462000155821443 0.015583999920636415 0.015583999920636415 0.015705999918282032 0.015706999925896525 0.01570700015872717 0.015746999997645617 0.015868999995291233 0.015909000067040324 0.015910000074654818 0.01599099999293685 0.016032000072300434 0.01607199991121888 0.01619499991647899 0.016235999995842576 0.016275999834761024 0.016438999911770225 0.016519999830052257 0.016682999907061458 0.016682999907061458 0.0166830001398921 0.0166830001398921 0.016723999986425042 0.016804999904707074 0.016967999981716275 0.017007999820634723 0.017048999899998307 0.0170889999717474 0.017089999979361892 0.017171000130474567 0.01721199997700751 0.017333999974653125 0.017333999974653125 0.017374000046402216 0.017374000046402216 0.01737500005401671 0.017414999892935157 0.01745599997229874 0.01745599997229874 0.017577999969944358 0.01761800004169345 0.017619000049307942 0.017740000039339066 0.0179449999704957 0.01810700003989041 0.018309999955818057 0.018473000032827258 0.01863699988462031 0.018757999874651432

@github-actions
Copy link
Contributor

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.92-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.92-4 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by https://github.com/thienlnam in version: 1.3.93-0 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by https://github.com/Beamanator in version: 1.3.93-1 🚀

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.

6 participants