@W-17443086 Allow store to be selected by Shopper#2187
Conversation
…b.com:SalesforceCommerceCloud/pwa-kit into W-17443086-select-store-from-store-locator-ui
| {store.city}, {store.stateCode ? store.stateCode : ''}{' '} | ||
| {store.postalCode} | ||
| </Box> | ||
| {store.distance !== undefined ? ( |
There was a problem hiding this comment.
Nit: Could these conditionals be simplified
store.distance !== undefined && ( ... )
There was a problem hiding this comment.
@hajinsuha1 I have a strong preference that we include a screenshot of "before" and "after" a given change for those changes that modify UI. It's a good "audit trail" to be able to look at a PR (when I look at git history) and see what the intent was behind a given change... do you mind adding screenshots of "before" and "after"?
| * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
| */ | ||
|
|
||
| import React from 'react' |
There was a problem hiding this comment.
You're missing a CHANGELOG.md update associated with this PR
There was a problem hiding this comment.
Added! @bfeister question on the release process. Currently I have the base branch set to a feature branch feature-bopis. Does this make sense or should I just merge the change into develop?
This change does works on it's own, although it doesn't really do anything other than save the store info in local storage
There was a problem hiding this comment.
I think it's fine to merge into develop since it works on it's own. It'll help to avoid a "big bang" merge and make that diff smaller
|
FYI: While making the CI pass, you may also find that some tests unrelated to your changes fail the pipeline. Some fixes were pushed recently to |
packages/template-retail-react-app/app/components/store-locator-modal/stores-list.jsx
Outdated
Show resolved
Hide resolved
| <AccordionPanel mb={6} mt={4}> | ||
| <div | ||
| dangerouslySetInnerHTML={{ | ||
| __html: store?.storeHours |
There was a problem hiding this comment.
Why dangerouslySetInnerHTML here? Can't we just interpolate the value in the place where we declare vars via useState and similar?
There was a problem hiding this comment.
The /store-search API actually returns the store hours as html
There was a problem hiding this comment.
Oh wow, SUPER weird, but ok 🚢
…b.com:SalesforceCommerceCloud/pwa-kit into W-17443086-select-store-from-store-locator-ui
| <HStack align="flex-start" mt="16px" mb="16px"> | ||
| <Radio | ||
| value={store.id} | ||
| mt="1.5px" |
There was a problem hiding this comment.
(non-blocking) weird to have 1.5px which seems like not a valid value. Reminds me of when Retina displays first came out and there were weird hacks like ~10 years ago. But if it's correct, then fine
There was a problem hiding this comment.
ooh good callout! So dug into this a bit, seems like all modern browsers support this while older browsers may round to the nearest whole number.
I also don't see anywhere else in PWA Kit that uses this so just for consistency i'll change it to 1px. UI wise this puts the radio button bottom-aligned with the store name rather than centered which isn't a big deal
hajinsuha1
dismissed stale reviews from jeremy-jung1 and bfeister
via
33a631f
3 participants
Description
Allows users to select a store in StoreLocator. Once a store is selected, the store id, name, and inventoryId are saved in local storage under the key
store_${siteId}.Before:
After:
Types of Changes
Changes
How to Test-Drive This PR
United States, Postal Code:94086, and press "Find" to see list of storesstore_RefArchGlobalwith a JSON value that contains the store id, store name, and store inventoryId of the selected store/store-searchAPIA11Y
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization