Migrate store locator from feature branch to develop#2542
Conversation
kevinxh
added
the
skip changelog
🎉 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) |
There was a problem hiding this comment.
I copied this file from the template-typescript-minimal project as I'm introducing typescript files to the retail react app.
| countryCode: 'DE', | ||
| countryName: 'Germany' | ||
| } | ||
| ] |
There was a problem hiding this comment.
How are the supported countries determined?
There was a problem hiding this comment.
The demo instance is set up to have these demo stores. We expect customers to change these values.
| export const STORE_LOCATOR_DEFAULT_COUNTRY = 'DE' | ||
| export const STORE_LOCATOR_DEFAULT_COUNTRY_CODE = 'DE' | ||
| export const STORE_LOCATOR_DEFAULT_PAGE_SIZE = 10 | ||
| export const STORE_LOCATOR_NUM_STORES_PER_REQUEST_API_MAX = 200 // This is an API limit and is therefore not configurable |
There was a problem hiding this comment.
These values seems to be configs, and we expect customers to tweak them? If so, can we rename them to config.js?
There was a problem hiding this comment.
Same comment as here #2542 (comment) ... my expectation is that since this V3 template is an upstream dependency for projects we want to avoid these kinds of big changes with only small value being provided since they will break customer implementations until the time when extensibility is removed altogether in V4
There was a problem hiding this comment.
I agree these are more of configs rather than constants. But I'm trying to avoid introducing new patterns in this PR. We can discuss how we want to make it easier to configure features in a separate discussion.
| @@ -135,11 +155,13 @@ export const TestProviders = ({ | |||
| fetchedToken={bypassAuth ? (isGuest ? guestToken : registerUserToken) : ''} | |||
| > | |||
| <CurrencyProvider currency={DEFAULT_CURRENCY}> | |||
There was a problem hiding this comment.
@bfeister and I talked about combining IntlProvider and CurrencyProvider - the latter is a part of the former and I don't see a need to have them seperated.
There was a problem hiding this comment.
@kzheng-sfdc I didn't realize we wanted to backport quite that much to V3. If we open the floodgates to that category of change (provider re-structure) there will be many more, but I'm not sure they provide value yet just because customers will see them as large + breaking changes that compete with the value add of the new functionality (the main thing they're after).
Open to being wrong about that
There was a problem hiding this comment.
Yeah let's hold off on making these breaking changes. I am just pointing out the TODOs for future v4 work.
|
Pending discussion with @kzheng-sfdc this looks good to me 👍 |
4 participants
Description
As we were exploring enhancing extensibility for PWA Kit, we experimented using the store locator as a standalone feature that can be optionally toggled/installed. It also gives us an opportunity to write modular code and split business logic from UI components.
In this PR, we are merging the optimized version back to develop branch.
Types of 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