Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
@W-13974769 - Show Color Attribute on ProductTile 🀄️ #1773
Changes from 22 commits
6de0fa7
3f52c3c
0561a0a
314f410
3dd556b
48e648b
958b72d
cfdc3fb
dc0cc9a
731c9ca
dd23007
328e951
c881f34
16996cd
703ac48
8b88143
eee2aaf
6f5a72d
7a08fe3
f6c6244
d3e9000
85027b7
2a216de
51c3ef1
3a59c8b
8fb7208
dc8dae0
a72613c
0c025d4
ca733fe
7694a50
e71cf8e
fa3506b
4412af4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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)?
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
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.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.
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
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.
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, ..."