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] Remove 'ExpensifyCashLogo.js' component #30241

Merged

Conversation

VickyStash
Copy link
Contributor

Details

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

Fixed Issues

$ #25029
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 24, 2023 15:49
@VickyStash VickyStash requested a review from a team as a code owner October 24, 2023 15:49
@melvin-bot melvin-bot bot requested review from ntdiary and removed request for a team October 24, 2023 15:50
@melvin-bot
Copy link

melvin-bot bot commented Oct 24, 2023

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

@ntdiary
Copy link
Contributor

ntdiary 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

Web image
Mobile Web - Chrome image
Mobile Web - Safari image
Desktop image
iOS image
Android image

Copy link
Contributor

@ntdiary ntdiary left a comment

Choose a reason for hiding this comment

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

image

ExpensifyCashLogo has been replaced by ExpensifyWordmark.
Also, It looks like the three icons new-expensify-adhoc.svg/new-expensify-dev.svg/new-expensify-stg.svg are no longer used either, but I'm not too sure if we should delete them.

@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 #25029 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 NikkiWines October 26, 2023 06:57
@roryabraham roryabraham changed the title [No QA] [TS migration] Migrate 'ExpensifyCashLogo.js' component to TypeScript [No QA] [TS migration] Remove 'ExpensifyCashLogo.js' component Oct 26, 2023
Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

👍

@NikkiWines NikkiWines merged commit 4809055 into Expensify:main Oct 26, 2023
16 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 1167.427 ms → 1233.733 ms (+66.306 ms, +5.7%) 🔴
Show details
Name Duration
App start TTI Baseline
Mean: 1167.427 ms
Stdev: 46.727 ms (4.0%)
Runs: 1068.807987999171 1077.241374000907 1095.1038959994912 1101.8224170003086 1106.4149029999971 1112.9902240000665 1113.8521829992533 1116.7398060001433 1117.4484540000558 1123.2711849994957 1123.400957999751 1126.1770050004125 1126.9230659995228 1127.2377899996936 1128.5591400004923 1128.86053000018 1133.5221090000123 1136.7529789991677 1138.0687060002238 1140.2730960007757 1140.3626669999212 1145.7134450003505 1146.5702030006796 1148.263267999515 1150.2460820004344 1150.5127140004188 1153.6326039992273 1156.3664970006794 1159.0302750002593 1159.9284259993583 1163.7912419997156 1164.6022050008178 1166.7085539996624 1173.5572989992797 1174.0677080005407 1174.1026729997247 1177.8104759994894 1179.0208950005472 1187.814837999642 1192.2358260005713 1192.3265729993582 1194.3076070006937 1195.3453540001065 1196.5240540001541 1200.1274159997702 1207.2382709998637 1209.7253239993006 1213.3675859998912 1215.610272999853 1217.9122640006244 1217.9411270003766 1222.5006440002471 1229.348660999909 1229.609872000292 1231.5839970000088 1240.276941999793 1243.3758770003915 1252.7368410006166 1253.6603800002486 1274.2963120006025

Current
Mean: 1233.733 ms
Stdev: 42.591 ms (3.5%)
Runs: 1132.2429799996316 1150.7532190000638 1151.7518619997427 1166.67380400002 1169.5528450002894 1169.9520600000396 1177.270533000119 1179.9364080000669 1192.4786499999464 1194.1770719997585 1197.2658670004457 1198.409881000407 1208.1247969996184 1208.2813449995592 1214.218205999583 1214.5522349998355 1216.6606869995594 1217.13262499962 1218.2619179999456 1219.1610719999298 1220.0175559995696 1222.5241240002215 1222.9958830000833 1225.02130900044 1225.573413000442 1226.3754300000146 1232.1767549999058 1233.9120739996433 1237.5857699997723 1237.9935809997842 1242.7856470001861 1243.5703969998285 1243.9141720002517 1246.4910559998825 1247.796256000176 1249.5716209998354 1250.3284360002726 1252.974766000174 1253.789916000329 1257.2736600004137 1257.590831999667 1258.8732629995793 1262.0037660002708 1262.2869459996 1264.2680500000715 1268.3328630002216 1268.7845270000398 1271.9154780004174 1281.973234999925 1283.818292000331 1286.7264109998941 1290.6627620002255 1295.3410099996254 1296.7552460003644 1333.6961319996044 1336.498855999671

Meaningless Changes To Duration

Show entries
Name Duration
App start runJsBundle 817.533 ms → 861.017 ms (+43.483 ms, +5.3%)
App start nativeLaunch 20.414 ms → 20.642 ms (+0.228 ms, +1.1%)
App start regularAppStart 0.014 ms → 0.014 ms (+0.000 ms, +3.2%)
Open Search Page TTI 723.441 ms → 718.992 ms (-4.449 ms, -0.6%)
Show details
Name Duration
App start runJsBundle Baseline
Mean: 817.533 ms
Stdev: 38.574 ms (4.7%)
Runs: 731 750 756 769 771 772 772 773 777 777 781 784 786 787 788 789 791 792 792 793 796 797 803 806 806 806 807 808 811 811 811 815 817 819 820 824 826 827 828 833 834 834 835 843 845 849 851 852 854 860 860 868 871 872 873 881 883 886 894 905

Current
Mean: 861.017 ms
Stdev: 40.512 ms (4.7%)
Runs: 771 781 782 788 802 803 815 817 819 819 822 825 826 830 832 834 837 838 842 843 846 849 850 851 856 859 859 859 861 862 862 864 866 866 867 869 870 872 872 876 876 877 884 887 888 888 891 894 896 897 899 900 908 915 919 922 926 929 941 962
App start nativeLaunch Baseline
Mean: 20.414 ms
Stdev: 1.752 ms (8.6%)
Runs: 18 18 19 19 19 19 19 19 19 19 19 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 21 21 21 21 21 21 21 21 22 22 23 23 23 23 23 24 24 24 25 25

Current
Mean: 20.642 ms
Stdev: 1.672 ms (8.1%)
Runs: 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 20 20 20 20 20 20 21 21 21 21 21 21 21 21 22 22 22 22 22 22 22 22 22 22 22 23 23 24 24 24 25
App start regularAppStart Baseline
Mean: 0.014 ms
Stdev: 0.001 ms (5.9%)
Runs: 0.012411000207066536 0.012574000284075737 0.012654000893235207 0.01269499957561493 0.012695999816060066 0.012695999816060066 0.012817999348044395 0.01285799965262413 0.012898001819849014 0.012980001047253609 0.013020999729633331 0.01302100159227848 0.01314300112426281 0.01314300112426281 0.013224000111222267 0.01326499879360199 0.013265000656247139 0.013304999098181725 0.01330600120127201 0.013345999643206596 0.013345999643206596 0.013345999643206596 0.013346999883651733 0.013387000188231468 0.013468001037836075 0.013468001037836075 0.013468999415636063 0.013508999720215797 0.013508999720215797 0.01355000026524067 0.013590000569820404 0.013590998947620392 0.013671999797224998 0.013671999797224998 0.013712998479604721 0.01371300034224987 0.013752998784184456 0.013957001268863678 0.013996999710798264 0.013996999710798264 0.0139979999512434 0.014038000255823135 0.014118999242782593 0.014159999787807465 0.014240998774766922 0.014322999864816666 0.014444999396800995 0.014606999233365059 0.014689000323414803 0.014770999550819397 0.014852000400424004 0.014852000400424004 0.015055999159812927 0.015258999541401863 0.015259001404047012 0.015298999845981598 0.015502000227570534 0.01586899906396866

Current
Mean: 0.014 ms
Stdev: 0.001 ms (4.8%)
Runs: 0.012654999271035194 0.013061999343335629 0.013143000192940235 0.013143000192940235 0.013182999566197395 0.013264999724924564 0.013345999643206596 0.013427999801933765 0.013469000346958637 0.013509999960660934 0.01355000026524067 0.013632000423967838 0.01371300034224987 0.013753000646829605 0.013753999955952168 0.013753999955952168 0.013793999329209328 0.0138349998742342 0.0138349998742342 0.013875000178813934 0.013876000419259071 0.013876000419259071 0.013915999792516232 0.013996999710798264 0.014038000255823135 0.014038000255823135 0.014038000255823135 0.014078999869525433 0.014078999869525433 0.014119000174105167 0.014119000174105167 0.014200999401509762 0.014241999946534634 0.014241999946534634 0.014241999946534634 0.014281999319791794 0.014282000251114368 0.014283000491559505 0.014283000491559505 0.0143630001693964 0.014444999396800995 0.01444500032812357 0.014648000709712505 0.014649000018835068 0.014688999392092228 0.014728999696671963 0.014728999696671963 0.0147299999371171 0.014810999855399132 0.01501499954611063 0.015015000477433205 0.015096000395715237 0.015135999768972397 0.015137000009417534 0.01534000039100647 0.015584000386297703 0.015584999695420265 0.015828999690711498
Open Search Page TTI Baseline
Mean: 723.441 ms
Stdev: 53.277 ms (7.4%)
Runs: 639.9823810011148 644.2910159993917 652.3183190003037 652.8276780005544 658.6005869992077 659.1781009994447 661.7736409995705 667.3031820002943 671.2512209992856 671.625489000231 672.1926279999316 673.3466800004244 673.4411210007966 674.5304769985378 677.2469490002841 680.7754730004817 681.7131350003183 681.8895270004869 682.3736170008779 683.9400229994208 687.2139079999179 688.4608159996569 694.3081459999084 696.6046550013125 696.6991789992899 698.0721840001643 699.4954840000719 699.6367189995944 706.5299889985472 707.6538899987936 718.272950001061 724.8663330003619 732.0533460006118 733.8240559995174 736.5371499992907 738.8302810005844 739.1387939993292 739.8044440001249 739.9916590005159 740.2185879983008 740.8606369998306 742.469280000776 744.8184410016984 749.366374000907 751.0275069996715 758.2633060012013 760.0419520009309 763.7047940008342 766.4590259995311 777.3174649998546 780.5529789999127 781.7635909989476 789.7613939996809 792.1925050001591 796.613160001114 806.172444999218 808.1728930007666 810.6714690010995 822.233846001327 828.1799730006605 880.4683019984514

Current
Mean: 718.992 ms
Stdev: 47.696 ms (6.6%)
Runs: 640.8018389996141 643.7781990002841 644.5124509995803 649.9458010001108 657.4871020000428 658.6747639998794 662.9265140006319 663.8447679998353 665.8974609998986 665.9872639998794 667.2855630004779 669.8098150007427 673.6964120008051 677.1583259999752 681.0848799999803 681.3221439998597 682.1684170002118 683.0805259998888 683.3830570001155 686.4399009998888 690.2111010001972 691.5931799998507 694.9369299998507 697.1684159999713 701.5022789994255 703.1408689999953 706.5024009998888 710.4873869996518 716.4652510005981 716.8736169999465 718.3005790002644 719.316487999633 719.498576999642 722.8436290007085 725.1431480003521 727.3170579997823 729.488567000255 730.6988530000672 732.7437349995598 735.6925049992278 737.4254970001057 738.3732099998742 738.3821620000526 748.9958089999855 752.1126310005784 752.4972339998931 753.6746020000428 754.853433999233 757.2552490001544 762.2510169995949 772.8887129994109 776.4115800000727 777.42834499944 778.9737139996141 784.3200289998204 786.0924079995602 793.250041000545 794.0034999996424 803.3588870000094 832.115396999754 834.6682940004393

@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/NikkiWines 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/NikkiWines 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.

None yet

5 participants