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

[N6.1] Use Google AddressSearch component in VBA Company Step #5643

Merged
merged 30 commits into from
Oct 7, 2021

Conversation

Luke9389
Copy link
Contributor

@Luke9389 Luke9389 commented Oct 4, 2021

Details

Fixed Issues

Half of https://github.com/Expensify/Expensify/issues/179233

Tests

  1. Head on over to new.expensify.com.dev or https://staging.new.expensify.com/
  2. Make a new account
  3. Validate that account
    a) If you're on staging, check your email for the validation link
    b) If you're on dev, run Expensidev/script/clitools.sh validator:account to validate the new account
  4. The Create Menu should pop up at the lower left, but if it doesn't click the green plus button down there
  5. Click 'New Workspace' (then click 'Expensify Card' if you're on mobile)
  6. Click 'Get Started' in the modal that pops up
  7. Click 'Connect Manually' in the VBA modal that comes up on the right hand side
  8. Use the following info for the first step
    a) Routing number 011401533
    b) Account number 1111222233331111
  9. Click 'Save & Continue'
  10. Type 42 in the 'Company Address' field
  11. Wait a sec <--- might want to look into this. The response time can be long.
  12. Verify that the address suggestions appear.
  13. Click one of them and verify that it enters that data into the 'Company Address' field

Tested On

  • Web
  • Mobile Web <--- finicky at best
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-10-04 at 2 21 09 PM

Screen Shot 2021-10-04 at 2 24 38 PM

Mobile

Web

Screen Shot 2021-10-04 at 2 31 28 PM

Desktop

Screen Shot 2021-10-04 at 2 34 07 PM

Screen Shot 2021-10-04 at 2 34 20 PM

iOS

Screen Shot 2021-10-04 at 3 00 00 PM

Screen Shot 2021-10-04 at 3 00 08 PM

We are getting an error here that we should not use VirtualizedLists inside of a scroll view (the component provided by this new npm package uses a VirtualizedList). Here is the issue that was opened in the repo of the npm package. Looking into potential solutions for this. FaridSafi/react-native-google-places-autocomplete#475 (comment)

Android

Screen Shot 2021-10-04 at 5 49 26 PM

Screen Shot 2021-10-04 at 5 49 37 PM

@Luke9389 Luke9389 requested a review from a team as a code owner October 4, 2021 22:03
@botify
Copy link

botify commented Oct 4, 2021

Hey, I noticed you changed some webpack configuration files. This can break production builds. Did you remember to run a production build locally to verify they still work?

@MelvinBot MelvinBot requested review from thienlnam and removed request for a team October 4, 2021 22:04
@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 4, 2021

still testing android. It's been a real saga with that today

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 4, 2021

Android tests worked

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 5, 2021

I think we can safely ignore the error that's thrown on mobile.
Here's the error:

VirtualizedLists should never be nested inside plain ScrollViews with the same orientation - use another VirtualizedList-backed container instead.

It's warning us about scrolling, but we're not displaying enough items in the dropdown list to be able to scroll. It feels... dicey to me to just straight up ignore this, but I wasn't able to break scrolling during any of my mobile tests.

@shawnborton
Copy link
Contributor

@Luke9389 let us know when you are ready for some design feedback too :)

@tgolen
Copy link
Contributor

tgolen commented Oct 5, 2021

@shawnborton I'm not sure if you saw it, but I wanted to be sure you read my comment about some of the styling issues I ran into. I can give more details, but it will take you very deep down several rabbit-holes.

src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
src/components/AddressSearch.js Outdated Show resolved Hide resolved
@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 5, 2021

After some 1:1 discussion @tgolen and I landed on a few things:

  1. The error I mentioned above isn't a pressing concern in the short term, so we're going to ignore it. In the long term, we'd like to fork the lib and follow the error message's recommendation to use a VirtualizedList-backed container rather than a VirtualizedList
  2. We can and should switch this to a class component to stay consistent with the rest of our codebase.

Commits for both to come soon after I retest on all platforms.

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 5, 2021

Also @shawnborton hit me with your style comments any time!

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 6, 2021

Ok so here's the new iteration. I've done away with the hard-coded width on the rows. I've tried (for longer than I'd like to admit) to get these elements to display the ... on the text-overflow, but haven't been able to do it.

Here are my investigation notes in case you'd like to take a crack at this @shawnborton.

OK, so we get to apply styles to the <GooglePlacesAutocomplete/> component in two places: the dropdown container, and the individual rows.

Here are those elements:

Dropdown Container
Screen Shot 2021-10-06 at 5 12 21 PM

Individual Row (one such row)
Screen Shot 2021-10-06 at 5 12 47 PM

The problem seems to be twofold:

  1. We can't apply styles to the elements in between (unless we want to fork this package)
  2. We seem to apply flex-drection: row to every view element (still not too sure about this one, but when I change the rule it fixes the problem, but I see a bunch of other elements on the page change as well).

Screen Shot 2021-10-06 at 5 12 34 PM

Screen Shot 2021-10-06 at 5 12 37 PM

I'm not sure how to get around this, so my current plan is to just try and get this merged now, and then we can continue taking a crack at adding the overflow ellipses when we have more time (N6.1 is Oct 15th). I think this is non-essential to complete before then (it works just fine on the other platforms right now).

What do you think Shawn & Jack? Here's what this looks like on mobile

iOS Android
Screen Shot 2021-10-06 at 5 08 08 PM Screen Shot 2021-10-06 at 5 12 01 PM

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 7, 2021

Updated and ready for review

@shawnborton
Copy link
Contributor

Sounds good to me re: investigating in a follow up issue, thanks for explaining. Could we also make the dropdown menu appear closer to the text input? I think I have it at 4px away in my mockup here:
image

thienlnam
thienlnam previously approved these changes Oct 7, 2021
@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 7, 2021

Updated. Decreased margin size to 4 px. Here's the most recent test shot

Screen Shot 2021-10-07 at 11 01 04 AM

@Luke9389 Luke9389 merged commit 8b81825 into main Oct 7, 2021
@Luke9389 Luke9389 deleted the tgolen-google-address-lookup branch October 7, 2021 19:11
@OSBotify
Copy link
Contributor

OSBotify commented Oct 7, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Oct 7, 2021

🚀 Deployed to staging by @Luke9389 in version: 1.1.6-1 🚀

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

@isagoico
Copy link

isagoico commented Oct 8, 2021

This issue is failing this PR #5724

@Luke9389
Copy link
Contributor Author

Luke9389 commented Oct 8, 2021

Hmm. That didn't happen in my testing. I'll take a look at this

@isagoico
Copy link

isagoico commented Oct 8, 2021

It worked! 🎉 Checking it off.
image

@marcaaron
Copy link
Contributor

Just a heads up we previously had error text displaying under the address field and now there's nothing

2021-10-12_13-31-04

tagging in @kevinksullivan @MitchExpensify in case this isn't on their radar.

@Luke9389
Copy link
Contributor Author

I'll whip up a PR for that, thanks @marcaaron

@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 ✅

@s77rt
Copy link
Contributor

s77rt commented Apr 20, 2023

This PR caused a bug #17277 When adding rounded corners to containers make sure the children does not get cut.

@s77rt
Copy link
Contributor

s77rt commented May 9, 2023

This PR caused a bug #16367. The query object parameter should have been memorised instead of creating a new one on every render. This is because the lib relays on objects equality not to "clear" the _requests array i.e passing a new object will clear the array. If we clear the array on every render there won't be any requests to cancel so the UI may show many successive results instead of just the last one.

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.

9 participants