-
Notifications
You must be signed in to change notification settings - Fork 125
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
@W-13974769 - Show Color Attribute on ProductTile 🀄️ #1773
Conversation
import {useCurrency} from '@salesforce/retail-react-app/app/hooks' | ||
import {filterImageGroups} from '../../utils/product-utils' |
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 created this little util as I saw that we were doing similar logic in other places. Thats why you'll see that I've changed another hook to use this util too.
isFavourite, | ||
onFavouriteToggle, | ||
dynamicImageProps, | ||
product, | ||
selectableAttributeId = 'color', |
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'm planning on adding this to the config file.. or maybe just the constant, and apply it from the product list component. Unless you have other ideas on how you think we might want to do this.
const initialVariationValue = isMasterVariant | ||
? product?.variants?.find((variant) => variant.productId == product.representedProduct.id) | ||
?.variationValues?.[selectableAttributeId] | ||
: undefined |
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 is where we use the representedProduct
to determine what the initial selected variation attribute value is.
?.variationValues?.[selectableAttributeId] | ||
: undefined | ||
|
||
const [selectableAttributeValue, setSelectableAttributeValue] = useState(initialVariationValue) |
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.
Question: I've been thinking about making this state and object instead of a single value. This might allow use some flexibility in the future as well as clean up some of the use's of that image utility that I created.
|
||
const {currency, image, price, productId, hitType} = product | ||
const isMasterVariant = ['master', 'variant'].includes(hitType) |
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.
Product Tile is used for both productSearch and einstein (which uses getProducts). The data may be a bit different when it comes to determine product type. We should consider for both cases here
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 looked at the recommended products on both the home page and the product details and the data looks like it's consistent to the point that this logic works. Do you know of any other specific uses where I'd want to check ?
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.
For Einstein recomendation, getProducts data does not have hitType instead it will be product.type.master or product.type.set
// NOTE: variationAttributes are only defined for master/variant type products. | ||
const variationAttributes = useMemo(() => { | ||
// NOTE: Decorate the product variant attributes to easily access images and hrefs. | ||
return product?.variationAttributes?.map((variationAttribute) => ({ |
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 logic here looks familiar with the useVariantionAttribute.js logic, can we extract or refactor that hook to accommodate both PLP and PDP. If not, can we extract this logic into it owns util function?
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.
So I made a utility that we can use in the future and use to refactor what we are already using in the hooks. At this time, that hook is too deeply engrained that I would prefer to do that work outside of this PR as it could possible bloat this PR to the point where the intended work gets hidden in a refactor.
minWidth="32px" | ||
backgroundRepeat="no-repeat" | ||
backgroundSize="cover" | ||
backgroundColor={name.toLowerCase()} |
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 assumes name is 'color', what happens if the name is different?
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 think what we should do here is select 'circle' as the only current option. I only say this because I don't want too much scope creep in making everything under the rainbow configurable. We can open it up for configurability after the initial version
packages/template-retail-react-app/app/components/product-tile/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/product-tile/index.jsx
Outdated
Show resolved
Hide resolved
packages/template-retail-react-app/app/components/product-tile/index.jsx
Outdated
Show resolved
Hide resolved
@@ -56,54 +65,135 @@ export const Skeleton = () => { | |||
* It also supports favourite products, controlled by a heart icon. | |||
*/ | |||
const ProductTile = (props) => { |
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.
Where are the updates to the unit tests for ProductTile (and SwatchGroup/Swatch)?
src={`${ | ||
image?.disBaseLink || | ||
image?.link || | ||
product?.image?.disBaseLink || | ||
product?.image?.disBaseLink | ||
}[?sw={width}&q=60]`} |
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.
duplicate value for product?.image?.disBaseLink || product?.image?.disBaseLink
@@ -14,6 +14,7 @@ import { | |||
useMultiStyleConfig | |||
} from '@salesforce/retail-react-app/app/components/shared/ui' | |||
import {Link as RouteLink} from 'react-router-dom' | |||
import {useBreakpointValue} from '@salesforce/retail-react-app/app/components/shared/ui' | |||
|
|||
/** | |||
* The Swatch Component displays item inside `SwatchGroup`. For proper keyboard accessibility, |
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 believe you had done some work for accessibility in this PR. How should we test-drive this change? Thanks.
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 re-implemented the existing logic we had to be more react-y. It didn't look like there were any tests for it tho. I've added some basic tests for using arrow keys to select swatches.
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.
Oh I meant manual tests.. how should we manually test the accessibility changes? I'm not familiar with how we did that last time.
packages/template-retail-react-app/app/components/product-tile/index.jsx
Outdated
Show resolved
Hide resolved
@@ -26,6 +26,14 @@ export const RECENT_SEARCH_LIMIT = 5 | |||
export const RECENT_SEARCH_KEY = 'recent-search-key' | |||
export const RECENT_SEARCH_MIN_LENGTH = 3 | |||
|
|||
// Constants for product list page | |||
export const PRODUCT_LIST_IMAGE_VIEW_TYPE = 'large' |
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.
Should we make it into an object? e.g
const PRODUCT_LIST = {
IMAGE_VIEW_TYPE: 'large',
SELECTED_ATTR: 'color'
}
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.
Here I'm following the current pattern that was established in the constants above it for search.. looks like page-config-value
. I think this is perfectly fine, if we decide to rework configuration (which I think we should) we'll probably address it in a broader scope there.
{variationAttributes | ||
?.filter(({id}) => selectableAttributeId === id) | ||
?.map(({id, values}) => ( | ||
<SwatchGroup |
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.
Nit: For a11y, we should make sure the attribute name (color/size) is included in the label for swatch. This is because not all color names are easy to regconize (e.g like seagrass or Light Seabreeze), without the context seeing them as colors, it may be hard for screen reader users to know what it is actually about.
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.
Good catch, it looks like we were overloading the "label" prop to be used for both the label, and the aria-label. As I didn't need or want the label to be displayed in this scenario I didn't provide a the label to the swatch group. This as expected stopped the label from showing, but also meant there was no aria-label.
So resolve this I've added an explicit "ariaLabel" prop that will take precedence over the label when passed in. This also won't effect the showing or hiding of the visual label.
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.
@bendvc the name here is still just the color value. The screen reader will says something like this "Seagrass, selected, radiogroup, 1 out of 2". It still misses the context of attribution type. I think it will be more helpful to have it like this so the screen will read out loud "Color Seagrass, select, ..."
ariaLabel={`${selectableAttributeId}-${name}`}
packages/template-retail-react-app/app/components/product-tile/index.jsx
Show resolved
Hide resolved
…/index.jsx Co-authored-by: Vincent Marta <vmarta@salesforce.com> Signed-off-by: Ben Chypak <bchypak@mobify.com>
</Box> | ||
|
||
{/* Swatches */} |
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.
question on the design - Can we make SwatchGroup a more independent component by passing all the required information to it? In this case it looks like it needs product to be passed may be as a prop.
This way we can move any code/logic specific to swatches into the SwatchGroup (Example: const variationAttributes = useMemo(() => getDecoratedVariationAttributes(product), [product])
. It is also easy for the users to extend/overide the component as it has all the information. And product tile will also be clean.
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.
These components are designed to be "dumb" lets say. You can use them to display anything you want really, it doesn't have to be product information at all. This is why you don't see that kind of logic inside the component definition.
packages/template-retail-react-app/app/components/product-tile/index.jsx
Outdated
Show resolved
Hide resolved
…/index.jsx Signed-off-by: Ben Chypak <bchypak@mobify.com>
Description
In this PR we enhance the ProductTile component to show a selectable attribute in the form of a swatch group. By default this attribute is "color" but can be changed by updating the constant file. This will allow the customer to view different colors of any given master/variant product type before clicking into the product detail page.
Types of Changes
Changes
How to Test-Drive This PR
npm run start
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization