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 7 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.
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.
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.
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.
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.
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.
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.
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
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.