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

Close AddressSearch results on blur #5832

Merged
merged 3 commits into from
Oct 14, 2021

Conversation

aldo-expensify
Copy link
Contributor

@aldo-expensify aldo-expensify commented Oct 13, 2021

Details

  • Remove keepResultsAfterBlur prop from GooglePlacesAutocomplete so it closes on blur
  • Pass down event to onBlur and onFocus in BaseExpensiTextInput. GooglePlacesAutocomplete will pass an onBlur callback that checks if the focus is changing to a result on blur. If we don't pass the event parameter, it fails to check that and closes the results list too soon.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/181023

Tests / QA

  1. Sign in to New Dot with a new account
  2. Create a new workspace
  3. Connect a new bank account
    Routing number: 123123123
    Account Number: 1111222233331111
  4. Hit enter, you should get to the Company Information page
  5. Start typing a Company address, you should see some suggested addresses appearing.
  6. Don't click any, click another field.
  7. The list of suggested addresses should disappear (close on blur).
  8. Start typing again in the Company address field and select one of the results
  9. The result list should disappear and the Company address should have the value of the selected result.

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen.Recording.2021-10-13.at.3.45.03.PM.mov

Mobile Web

Desktop

iOS

Android

@aldo-expensify aldo-expensify requested a review from a team as a code owner October 13, 2021 22:43
@MelvinBot MelvinBot requested review from marcochavezf and removed request for a team October 13, 2021 22:43
@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Oct 13, 2021

Should I put CP-Staging in this?

@aldo-expensify aldo-expensify self-assigned this Oct 13, 2021
@Luke9389
Copy link
Contributor

Nice! It looks like this fixes the gray bar under the field too right?

Copy link
Contributor

@Luke9389 Luke9389 left a comment

Choose a reason for hiding this comment

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

Would you be sure to test this on the date picker as well? We pass an onFocus prop there and I just want to double-check that it works still.

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Oct 14, 2021

Nice! It looks like this fixes the gray bar under the field too right?

hmm not exactly.. the gray bar appear less now because we close the results on blur, but you will still see it if you do:

  1. type something in Company address
  2. select result
  3. click the Company address field again (focus)

The gray bar will be there because it is focused and there are no results :(

@aldo-expensify
Copy link
Contributor Author

aldo-expensify commented Oct 14, 2021

Would you be sure to test this on the date picker as well? We pass an onFocus prop there and I just want to double-check that it works still.

I didn't test it there, I'll do it now.

@Luke9389
Copy link
Contributor

But it's good that the gray bar is showing far less often!

@aldo-expensify
Copy link
Contributor Author

But it's good that the gray bar is showing far less often!

@aldo-expensify
Copy link
Contributor Author

Would you be sure to test this on the date picker as well? We pass an onFocus prop there and I just want to double-check that it works still.

Played with the Date picker, it looks fine 🤞

@github-actions
Copy link
Contributor

⚠️ ⚠️ Heads up! This pull request has the CP Staging label. ⚠️ ⚠️
Merging it will cause it to be immediately deployed to staging, even if the open StagingDeployCash deploy checklist is locked.

@Luke9389
Copy link
Contributor

K, I'm testing on android rigtht now. Wanna do iOS?

@aldo-expensify
Copy link
Contributor Author

K, I'm testing on android rigtht now. Wanna do iOS?

sure!

@aldo-expensify
Copy link
Contributor Author

There is this ugly bug when we reload the page during the Company step:

Screen.Recording.2021-10-13.at.5.21.35.PM.mov

but I think that was there from before and we should make a different GH issue for it. I would think it is probably related to how we store/read the draft? info.

@Luke9389
Copy link
Contributor

Worked on android 🎉
Screen Shot 2021-10-13 at 5 23 39 PM

@aldo-expensify
Copy link
Contributor Author

Worked in IOS 🎉

Screen.Recording.2021-10-13.at.5.38.21.PM.mov

@Luke9389 Luke9389 merged commit 825977a into main Oct 14, 2021
@Luke9389 Luke9389 deleted the aldo_fix-address-search-close-on-blur branch October 14, 2021 01:10
github-actions bot pushed a commit that referenced this pull request Oct 14, 2021
…-on-blur

Close AddressSearch results on blur

(cherry picked from commit 825977a)
@OSBotify
Copy link
Contributor

🚀 Cherry-picked to staging by @Luke9389 in version: 1.1.7-15 🚀

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

@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes.

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to staging by @Luke9389 in version: 1.1.7-25 🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @roryabraham in version: 1.1.8-9 🚀

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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants