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.

Sign up for GitHub

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

Jump to bottom

Fix: Address suggestion lists hover and selection color issue #14724

Merged
merged 3 commits into from
Feb 6, 2023
Merged

Fix: Address suggestion lists hover and selection color issue #14724

merged 3 commits into from
Feb 6, 2023

Conversation

situchan
Copy link
Contributor

@situchan situchan commented Feb 1, 2023 •
edited
Loading

Details

In react-native-google-places-autocomplete repo:

  • replace TouchableHighlight with Pressable
  • introduce new prop called listHoverColor

In E/App repo:

  • pass listHoverColor, listUnderlayColor prop values to GooglePlacesAutocomplete component

Fixed Issues

$ #13838
PROPOSAL: #13838 (comment)

Tests

  1. Go to Settings -> Workspaces -> select workspace
  2. Choose "Connect bank account" -> "Connect manually"
  3. Go to Step 2 of the Form
  4. Write any value in "Company address" input to get items
  5. Hover any item and verify that background color matches our dark theme
  6. Press any item and hold to verify that background color matches our dark theme
  • Verify that no errors appear in the JS console

Offline tests

"Connect bank account" is not available in offline mode

QA Steps

  1. Go to Settings -> Workspaces -> select workspace
  2. Choose "Connect bank account" -> "Connect manually"
  3. Go to Step 2 of the Form
  4. Write any value in "Company address" input to get items
  5. Hover any item and verify that background color matches our dark theme
  6. Press any item and hold to verify that background color matches our dark theme
  • Verify that no errors appear in the JS console

PR Author Checklist

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

Screenshots/Videos

Web
web.mov
Mobile Web - Chrome
mchrome.mov
Mobile Web - Safari
msafari.mp4
Desktop
desktop.mov
iOS
ios.mp4
Android
android.mov

@situchan situchan requested a review from a team as a code owner February 1, 2023 08:10
@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

@melvin-bot melvin-bot bot requested review from grgia and removed request for a team February 1, 2023 08:10
@melvin-bot
Copy link

melvin-bot bot commented Feb 1, 2023

@grgia @Santhosh-Sellavel 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]

@situchan situchan mentioned this pull request Feb 1, 2023
Closed
2 tasks
@Santhosh-Sellavel
Copy link
Collaborator

@situchan please mark this as a draft until the other one is merged!

@situchan situchan marked this pull request as draft February 1, 2023 20:21
@situchan situchan marked this pull request as ready for review February 2, 2023 20:55
@situchan
Copy link
Contributor Author

situchan commented Feb 2, 2023

@grgia @Santhosh-Sellavel updated library version. now ready for review

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 2, 2023 •
edited
Loading

@situchan Please make PR title meaningful

@situchan situchan changed the title set custom hover, pressed color on google places autocomplete list update hover, pressed background color on company address suggestions list for dark mode Feb 2, 2023
@situchan
Copy link
Contributor Author

situchan commented Feb 2, 2023

@situchan Please make PR title meaningful

@Santhosh-Sellavel updated. looks good now?

@Santhosh-Sellavel
Copy link
Collaborator

Santhosh-Sellavel commented Feb 2, 2023 •
edited
Loading

Reviewer Checklist

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

Screenshots/Videos

Web
Screen.Recording.2023-02-03.at.3.32.00.AM.mov
Mobile Web - Chrome
Screen.Recording.2023-02-03.at.3.34.28.AM.mov
Mobile Web - Safari
Simulator.Screen.Recording.-.iPhone.14.-.2023-02-03.at.03.35.45.mp4
Desktop
Screen.Recording.2023-02-03.at.3.29.18.AM.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.-.2023-02-03.at.03.36.49.mp4
Android
Screen.Recording.2023-02-03.at.3.37.22.AM.mov

Santhosh-Sellavel
Santhosh-Sellavel approved these changes Feb 2, 2023
View reviewed changes
Copy link
Collaborator

@Santhosh-Sellavel Santhosh-Sellavel left a comment

Choose a reason for hiding this comment

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

LGTM, all you @grgia!

@Santhosh-Sellavel
Copy link
Collaborator

@situchan Please make PR title meaningful

@Santhosh-Sellavel updated. looks good now?

Could be better

Fix: Address suggestion lists hover and selection color issue

@situchan situchan changed the title update hover, pressed background color on company address suggestions list for dark mode Fix: Address suggestion lists hover and selection color issue Feb 2, 2023
@Santhosh-Sellavel
Copy link
Collaborator

@grgia bump!

@situchan
Copy link
Contributor Author

situchan commented Feb 4, 2023

@grgia kindly bump!

@grgia
Copy link
Contributor

grgia commented Feb 6, 2023

Thanks for the bump! Reviewing now

grgia
grgia approved these changes Feb 6, 2023
View reviewed changes
Copy link
Contributor

@grgia grgia left a comment

Choose a reason for hiding this comment

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

LGTM!

@grgia grgia merged commit d2f65e7 into Expensify:main Feb 6, 2023
@melvin-bot
Copy link

melvin-bot bot commented Feb 6, 2023

Congrats, that’s your 5th PR merged! 🎉 Do you know about the ContributorPlus role? It’s an opportunity to earn more in the Expensify Open Source community. Keep up the great work - thanks!

@OSBotify
Copy link
Contributor

OSBotify commented Feb 6, 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
Copy link
Contributor

github-actions bot commented Feb 6, 2023

Performance Comparison Report 📊

Significant Changes To Duration

There are no entries

Meaningless Changes To Duration

Show entries
Name Duration
App start TTI 692.626 ms → 727.392 ms (+34.766 ms, +5.0%)
App start runJsBundle 192.594 ms → 198.097 ms (+5.503 ms, +2.9%)
Open Search Page TTI 620.376 ms → 623.274 ms (+2.898 ms, ±0.0%)
App start regularAppStart 0.015 ms → 0.015 ms (+0.001 ms, +3.5%)
App start nativeLaunch 19.258 ms → 19.154 ms (-0.104 ms, -0.5%)
Show details
Name Duration
App start TTI Baseline
Mean: 692.626 ms
Stdev: 34.585 ms (5.0%)
Runs: 624.7872529998422 646.537484000437 647.6462190002203 650.486256999895 658.4569889996201 661.6581119997427 663.043240999803 665.2823510002345 669.6160709997639 671.168503000401 678.4632829995826 680.5136690000072 682.6985130002722 685.0590350003913 685.6546430001035 688.6808519996703 691.0299779996276 691.3812800003216 693.7092450000346 694.1146280001849 695.255029999651 698.7066869996488 702.4192580003291 706.3077980000526 714.9748360002413 725.9161940002814 731.3515219995752 735.5525200003758 740.6662550000474 746.5642830003053 751.8642239999026 784.4587110001594

Current
Mean: 727.392 ms
Stdev: 28.729 ms (3.9%)
Runs: 663.1159950001165 672.1792169995606 678.9600480003282 691.443237000145 700.1556339999661 704.08198300004 704.4622120000422 711.139577999711 712.4398290002719 714.5125200003386 716.7149069998413 718.0096749998629 720.8909750003368 721.1813899995759 723.8607029998675 724.2579229995608 726.0265669999644 728.8872520001605 736.6536720003933 738.0238640001044 743.9663380002603 745.6554749999195 749.7717310003936 751.749892000109 754.2499550003558 759.8077640002593 760.9049410000443 763.483114999719 763.9666959997267 765.4423909997568 783.1510579995811
App start runJsBundle Baseline
Mean: 192.594 ms
Stdev: 24.017 ms (12.5%)
Runs: 163 163 164 166 166 170 171 173 174 175 175 179 182 183 183 187 187 189 191 195 195 200 205 208 214 219 220 221 221 225 243 256

Current
Mean: 198.097 ms
Stdev: 14.623 ms (7.4%)
Runs: 166 173 175 178 184 185 188 188 189 191 192 193 193 194 196 198 201 202 202 204 204 206 209 210 211 212 214 214 219 221 229
Open Search Page TTI Baseline
Mean: 620.376 ms
Stdev: 22.475 ms (3.6%)
Runs: 578.418456999585 578.8948570005596 580.4233800005168 587.256347999908 591.8104659998789 600.3625489994884 602.7803960004821 603.8103029998019 606.9698489997536 608.0829269997776 608.6585699999705 608.9055589996278 614.9193110000342 615.2271320000291 619.9753419999033 620.5206300001591 622.4531259993091 624.3743090005592 624.55102599971 627.5310060000047 628.5163979995996 628.6417640000582 631.1020099995658 632.4488939996809 633.1138920001686 635.4453530004248 636.3913980005309 643.5980640007183 647.5122889997438 648.8488780008629 656.4615079993382 658.9204510003328 665.4679770004004

Current
Mean: 623.274 ms
Stdev: 21.797 ms (3.5%)
Runs: 585.5756029998884 588.085124000907 592.1697189994156 599.7311200005934 599.7738859998062 605.5233970005065 605.5751550002024 606.5393479997292 610.9649670002982 613.8025310002267 614.4222820000723 614.5078939991072 615.4116620002314 620.0852859998122 620.5912680001929 621.313395999372 621.6326500000432 622.537271999754 624.0509440004826 625.5692549999803 625.5792640000582 626.4634610004723 632.3416760005057 633.072835999541 633.9538580002263 634.9836840005592 639.091878999956 640.0895999995992 658.7588300006464 667.5329590002075 668.7088219998404 676.3145349994302
App start regularAppStart Baseline
Mean: 0.015 ms
Stdev: 0.001 ms (7.1%)
Runs: 0.012776000425219536 0.013020000420510769 0.013427999801933765 0.013631000183522701 0.013671999797224998 0.013712000101804733 0.01371300034224987 0.013753999955952168 0.013835000805556774 0.013996999710798264 0.013997000642120838 0.0139979999512434 0.014078999869525433 0.01416000071913004 0.01428299956023693 0.014322999864816666 0.014322999864816666 0.014364000409841537 0.014607000164687634 0.014688999392092228 0.014810999855399132 0.014973999932408333 0.015096000395715237 0.015421999618411064 0.015543999150395393 0.015584000386297703 0.015625 0.015665999613702297 0.015828999690711498 0.01631700061261654 0.01766000036150217

Current
Mean: 0.015 ms
Stdev: 0.001 ms (5.8%)
Runs: 0.013143000192940235 0.013671999797224998 0.013794000260531902 0.013835000805556774 0.014201000332832336 0.014240999706089497 0.014403999783098698 0.014444999396800995 0.014485999941825867 0.014689000323414803 0.0147299999371171 0.014851999469101429 0.014891999773681164 0.014892000705003738 0.0148930000141263 0.014932999387383461 0.014932999387383461 0.015015000477433205 0.015137000009417534 0.015300000086426735 0.015584000386297703 0.015665999613702297 0.015747000463306904 0.015747000463306904 0.015786999836564064 0.0157880000770092 0.015990999527275562 0.016153999604284763 0.016439000144600868 0.01660200022161007 0.01676500029861927
App start nativeLaunch Baseline
Mean: 19.258 ms
Stdev: 1.606 ms (8.3%)
Runs: 17 17 17 18 18 18 18 18 18 18 18 18 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21 22 22 23 23

Current
Mean: 19.154 ms
Stdev: 0.769 ms (4.0%)
Runs: 18 18 18 18 18 19 19 19 19 19 19 19 19 19 19 19 19 19 20 20 20 20 20 20 20 21

@OSBotify OSBotify mentioned this pull request Feb 6, 2023
Closed
6 tasks
@OSBotify
Copy link
Contributor

OSBotify commented Feb 6, 2023

🚀 Deployed to staging by https://github.com/grgia in version: 1.2.66-0 🚀

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

@OSBotify
Copy link
Contributor

OSBotify commented Feb 7, 2023

🚀 Deployed to production by https://github.com/mountiny in version: 1.2.66-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
Reviewers

@Santhosh-Sellavel Santhosh-Sellavel Santhosh-Sellavel approved these changes

@grgia grgia grgia approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@situchan @Santhosh-Sellavel @grgia @OSBotify