-
Notifications
You must be signed in to change notification settings - Fork 195
@W-19319212: Add Address Autocompletion feature branch #3071
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
base: develop
Are you sure you want to change the base?
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
…resses (#2614) This PR implements Address Autocomplete functionality for checkout in the PWA Kit storefront.The implementation includes a mock address service that simulates the Google Places API, which can then be replaced with the actual API integration in the future.
…2767) Added city, state/province, and zip/postal code field completion once suggestion is selected from dropdown
Added Google Places API integration for address autocompletion dropdown. Also created reusable hook for API call and updated tests
Modified dropdown to follow Figma design Signed-off-by: harshini-magesh <harshini.magesh@salesforce.com>
bd8593f
to
167c03f
Compare
Signed-off-by: d.phan <d.phan@salesforce.com>
Signed-off-by: d.phan <d.phan@salesforce.com>
Signed-off-by: d.phan <d.phan@salesforce.com>
Signed-off-by: d.phan <d.phan@salesforce.com>
Signed-off-by: Danny Phan <125327707+dannyphan2000@users.noreply.github.com>
// Process address suggestion using unified utility method | ||
const addressFields = await processAddressSuggestion(suggestion) | ||
|
||
// Use the utility function to set address fields |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove unnecessary comments
@@ -0,0 +1,364 @@ | |||
/* | |||
* Copyright (c) 2021, salesforce.com, inc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2025
*/ | ||
|
||
// Country code mapping for address parsing | ||
const COUNTRY_CODE_MAP = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file a WI for checkout team to use sites config instead
|
||
// Extract country code from the last term | ||
const countryTerm = terms[terms.length - 1]?.value || '' | ||
if (countryTerm === 'USA') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
addressFields['locality'] = parts[1] | ||
const lastPart = parts[2] | ||
|
||
if (lastPart === 'USA' || lastPart === 'Canada') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fix
Signed-off-by: d.phan <d.phan@salesforce.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a few nits on the tests but this mostly looks fine.
As discussed during our meet, the main thing I think we'll want to address is the fact that we're hard coding US and CA in a number of places. Ideally, we're using the countries defined for a site in sites.jsx as the source of which countries to include so that customer projects have a single place to configure when setting different countries for their site. Not all projects may have a US or CA locale so having a single place where they can change this would be good. At the moment, customer projects will need to modify several places (including the component and utils) if they wanted to remove US / CA.
But, as discussed, even prior to these changes, there are already hard coded address behavior for US / CA. Those too will need to be addressed. Let's have a follow up ticket to address this.
For now, seeing as there is precedent for hard coding US / CA, I'm willing to go with the current approach in the short term.
expect(result.current).toHaveProperty('preferred') | ||
}) | ||
|
||
it('should set default country to US', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default country may not always be US so keep in mind that this test may be something that will change when the follow up ticket to use the countries + default country or locale defined in sites.js.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our Slack convo, I'll remove these country-specific utests since eventually we want to move to a more flexible support model for different locales/countries in the checkout space
expect(mockOnChange).toHaveBeenCalledWith('1234567890') | ||
}) | ||
|
||
it('should show province label for Canada', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This and the other tests below are something that may get replaced by projects if they are not using US / CA locale.
I'm not sure we'll want these tests around once the follow up ticket is done but we'll have to do something about the hard coded behavior for CA (that was present before the changes in this PR) first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed and removed - my response above
await act(async () => { | ||
jest.advanceTimersByTime(350) | ||
await Promise.resolve() | ||
await Promise.resolve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call this code inside the act in a number of tests so I wonder if we could refactor it out so it's only defined once.
Also, why do some tests call Promise.resolve()
twice while others only once?
A comment explaining why we call Promise.resolve()
once or twice might be helpful
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great catch! Fixed with one Promise.resolved() and extracted to a reusable method
Signed-off-by: d.phan <d.phan@salesforce.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making the suggested changes @dannyphan2000 !
…n-merge-latest-aug28 Merge latest from develop branch
5bc0bc2
Merging address autocompletion feature to develop branch.
Description
Merging address autocompletion feature to develop branch. The changes included are from previously approved PRs.
#2614
#2767
#2886
#2981
Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization