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

Fix hover effect on request or send money messages #15551

Conversation

tienifr
Copy link
Contributor

@tienifr tienifr commented Feb 28, 2023

Details

The arrow icon color should consistent on hover in request / send money arrow.

Fixed Issues

$ #15368
PROPOSAL: #15368 (comment)

Tests

  1. Open the app
  2. Open any report
  3. Request or send money
  4. Hover on message
  5. Verify that the arrow icon color change to '#E7ECE9'
  • Verify that no errors appear in the JS console

Offline tests

QA Steps

  1. Open the app
  2. Open any report
  3. Request or send money
  4. Hover on message
  5. Verify that the arrow icon color change to '#E7ECE9'
  • 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.
  • 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

Web
Screen.Recording.2023-02-28.at.11.28.09.mov
Mobile Web - Chrome
RPReplay_Final1677559354.mp4
Mobile Web - Safari
RPReplay_Final1677559789.mp4
Desktop
Screen.Recording.2023-02-28.at.11.52.38.mov
iOS
Screen.Recording.2023-02-28.at.12.44.29.mov
Android
Screen.Recording.2023-02-28.at.13.01.08.mov

@tienifr tienifr marked this pull request as ready for review February 28, 2023 06:10
@tienifr tienifr requested a review from a team as a code owner February 28, 2023 06:10
@melvin-bot melvin-bot bot requested review from AndrewGable and sobitneupane and removed request for a team February 28, 2023 06:10
@MelvinBot
Copy link

@AndrewGable @sobitneupane One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button]

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

Screenshots/Videos

Web
Screen.Recording.2023-02-28.at.18.21.22.mov
Mobile Web - Chrome
Screen.Recording.2023-02-28.at.18.36.54.mov
Mobile Web - Safari
Screen.Recording.2023-02-28.at.18.24.29.mov
Desktop
Screen.Recording.2023-02-28.at.18.26.07.mov
iOS
Screen.Recording.2023-02-28.at.18.38.34.mov
Android
Screen.Recording.2023-02-28.at.18.37.42.mov

Copy link
Contributor

@sobitneupane sobitneupane left a comment

Choose a reason for hiding this comment

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

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

@AndrewGable AndrewGable merged commit 9407b82 into Expensify:main Feb 28, 2023
@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
Copy link
Contributor

Performance Comparison Report 📊

Significant Changes To Duration

Name Duration
Open Search Page TTI 609.078 ms → 671.567 ms (+62.489 ms, +10.3%) 🔴
Show details
Name Duration
Open Search Page TTI Baseline
Mean: 609.078 ms
Stdev: 29.403 ms (4.8%)
Runs: 570.782105000224 577.2310790000483 580.8649909999222 581.003703000024 581.3640139997005 583.6107989996672 586.9243980003521 587.837199000176 588.0380049999803 588.2730310000479 589.5029299999587 589.7896730001085 591.2430429998785 592.7010909998789 596.5379640003666 598.959309999831 600.3955890000798 601.9217940000817 602.9499920001253 603.9280199999921 606.2798669999465 610.769449999556 614.9242359995842 619.4034420000389 619.8721929998137 627.7899579997174 635.8743090000935 646.8896900000982 650.784750000108 652.3905839999206 654.1834720000625 680.8012689999305 685.7495120000094

Current
Mean: 671.567 ms
Stdev: 19.624 ms (2.9%)
Runs: 630.2305910000578 637.8568929997273 646.7786050001159 646.8765869997442 647.4535719999112 655.9829520001076 658.2875169999897 658.8746750000864 661.6241869996302 662.6534830001183 663.3847659998573 663.8113609999418 664.1204429999925 664.1792000001296 665.1446539997123 666.894125000108 670.6152759999968 671.4753419999033 671.9191089998931 674.5990810003132 679.2368169999681 679.4459230001085 680.3752039996907 681.8048910000362 686.0968419997953 689.829143000301 692.2275809999555 699.7323809997179 700.3500980003737 700.9346110001206 701.8748379996978 715.4587820000015

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 699.400 ms → 723.159 ms (+23.759 ms, +3.4%)
App start runJsBundle 191.125 ms → 198.469 ms (+7.344 ms, +3.8%)
App start nativeLaunch 18.893 ms → 19.207 ms (+0.314 ms, +1.7%)
App start regularAppStart 0.013 ms → 0.014 ms (+0.001 ms, +4.9%)
Show details
Name Duration
App start TTI Baseline
Mean: 699.400 ms
Stdev: 22.410 ms (3.2%)
Runs: 652.9205789999105 660.1671250001527 667.9973320001736 670.7632630001754 677.3383470000699 679.0313679999672 685.6544269998558 687.0542000001296 690.083509999793 692.4123970000073 693.6394440000877 694.2285119998269 696.1376379998401 697.9492080002092 698.5272840000689 698.7512559997849 702.8435920001939 702.9881930002011 705.1351789999753 706.3675919999368 707.0588699998334 709.7715130001307 713.9533029999584 715.2325450000353 719.2081160000525 732.8832220002078 736.5086860000156 742.5544280000031 745.4284870000556

Current
Mean: 723.159 ms
Stdev: 30.314 ms (4.2%)
Runs: 665.6336570000276 674.3248419999145 680.6800299999304 681.3714459999464 687.2522390000522 688.1200990001671 690.1026849998161 694.9387690001167 695.5206769998185 712.7342460001819 716.4600450000726 717.8033850002103 719.1511639999226 720.1712429998443 721.8518520002253 725.6203580000438 726.2281129998155 729.5350779998116 729.7373959999532 731.9323169998825 736.0779320001602 737.9478230001405 739.3846450001001 742.2970540001988 745.6098580001853 749.4166339999065 750.4805890000425 752.28216599999 752.8547419998795 755.0631240000948 760.8147060000338 809.6771559999324
App start runJsBundle Baseline
Mean: 191.125 ms
Stdev: 19.201 ms (10.0%)
Runs: 164 166 166 169 169 172 172 173 175 178 185 185 185 185 186 187 188 189 190 191 191 197 200 201 211 213 214 215 216 216 226 241

Current
Mean: 198.469 ms
Stdev: 18.429 ms (9.3%)
Runs: 174 175 175 176 177 177 180 181 182 182 185 186 188 191 194 197 198 200 202 205 206 208 209 211 213 220 220 222 223 229 230 235
App start nativeLaunch Baseline
Mean: 18.893 ms
Stdev: 1.263 ms (6.7%)
Runs: 17 17 17 17 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 22

Current
Mean: 19.207 ms
Stdev: 1.186 ms (6.2%)
Runs: 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 21 21 21 23
App start regularAppStart Baseline
Mean: 0.013 ms
Stdev: 0.001 ms (6.1%)
Runs: 0.011799999978393316 0.012165999971330166 0.012409999966621399 0.012451000045984983 0.012451000045984983 0.012451000045984983 0.012613999657332897 0.012614000122994184 0.012695999816060066 0.012735999654978514 0.012736000120639801 0.0129399998113513 0.013019999954849482 0.013061999808996916 0.013143000192940235 0.01318400027230382 0.013224000111222267 0.013224999886006117 0.0134680001065135 0.01355000026524067 0.01355000026524067 0.01355000026524067 0.013671999797224998 0.013672000262886286 0.013793999794870615 0.014120000414550304 0.014282000251114368 0.014323000330477953 0.014405000023543835 0.014688999857753515 0.014973999932408333 0.015096000395715237

Current
Mean: 0.014 ms
Stdev: 0.001 ms (6.0%)
Runs: 0.012369999662041664 0.012899000197649002 0.012980000115931034 0.013021000195294619 0.013061000034213066 0.01310200011357665 0.013142999727278948 0.013183999806642532 0.013305999804288149 0.013509000185877085 0.01355000026524067 0.013590000104159117 0.013711999636143446 0.013712999876588583 0.013712999876588583 0.013753000181168318 0.013835000339895487 0.013915999792516232 0.013915999792516232 0.014078999869525433 0.014160000253468752 0.014485999941825867 0.014526000246405602 0.014527000021189451 0.014567000325769186 0.014728999696671963 0.0147299999371171 0.014811000321060419 0.015054999850690365 0.015217999927699566 0.015746999997645617 0.015868999995291233

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

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 2, 2023

🚀 Deployed to staging by https://github.com/AndrewGable in version: 1.2.78-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Mar 6, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.78-0 🚀

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