-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Conversation
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? |
still testing android. It's been a real saga with that today |
Android tests worked |
I think we can safely ignore the error that's thrown on mobile.
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. |
@Luke9389 let us know when you are ready for some design feedback too :) |
@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. |
After some 1:1 discussion @tgolen and I landed on a few things:
Commits for both to come soon after I retest on all platforms. |
Also @shawnborton hit me with your style comments any time! |
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 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 Here are those elements: The problem seems to be twofold:
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
|
Updated and ready for review |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
This issue is failing this PR #5724 |
Hmm. That didn't happen in my testing. I'll take a look at this |
Just a heads up we previously had error text displaying under the address field and now there's nothing tagging in @kevinksullivan @MitchExpensify in case this isn't on their radar. |
I'll whip up a PR for that, thanks @marcaaron |
🚀 Deployed to production by @AndrewGable in version: 1.1.7-24 🚀
|
This PR caused a bug #17277 When adding rounded corners to containers make sure the children does not get cut. |
This PR caused a bug #16367. The |
Details
Fixed Issues
Half of https://github.com/Expensify/Expensify/issues/179233
Tests
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 accounta) Routing number 011401533
b) Account number 1111222233331111
42
in the 'Company Address' fieldTested On
Screenshots
Web
Mobile
Web
Desktop
iOS
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