-
Notifications
You must be signed in to change notification settings - Fork 2
[CI-4169] Adds the ability to override the image url. #134
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
base: main
Are you sure you want to change the base?
[CI-4169] Adds the ability to override the image url. #134
Conversation
mocca102
left a comment
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.
Types are failing here since the getters return string | undefined but string is expected
src/utils/itemFieldGetters.ts
Outdated
| // eslint-disable-next-line import/prefer-default-export | ||
| export function getPrice(item: Item | Variation): number { | ||
| return item.data.price; | ||
| export function getPrice(item: Item | Variation, selectedSwatch?: SwatchItem | undefined): number { |
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 see the getter getPrice here accepts an item and a selectedSwatch (variation), but ItemFieldGetters.getPrice gets only item.
Should the type of ItemFieldGetters.getPrice be updated? to accept item and variation? Currenlty it only accepts item of type Item OR Variation
same for ItemFieldGetters.getName, ItemFieldGetters.getImageUrl, ItemFieldGetters.getItemUrl
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 feel like selectedSwatch shouldn't be a valid argument here since it's already a transformed object. itemFieldGetters as a pattern should only care about parsing through the data we return in the API response and doing the minimum transformation to match the schema the PLP library expects.
That said, I kind of understand why we need it, since the logic for changing item_name, price, etc on swatch click is a client-side only interaction that needs to exist somewhere. Let me think on this for a bit.
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 wonder if we've too tightly coupled useProductInfo and useProductSwatch. Right now useProductSwatch is called within useProductInfo, but perhaps what we should've done is have them be entirely independent of each other.
As a recap, currently:
useProductSwatch
- Extracts fields from the Customer's
dataobject to generates aswatchListto render swatches - provides the function
selectVariationto toggle which swatch is selected - returns the reactive
selectedVariation
useProductInfo
- Extracts fields from the Customer's
dataobject and returns a set of fields that are relevant for the Product Card to render - Also calls
useProductSwatchand returns the output
We can see that we're doing field extraction in two separate locations which is a potential yellow flag. Maybe we need to do a little exercise in separation of concerns. What if we think of the two hooks as serving two separate complimentary roles:
useProductSwatch- the hook in charge of identifying and keeping track of which variation is selecteduseProductInfo- the hook in charge of parsing the Customer's uniquedataschema and transforming it into the schema the library expects
This seems to neatly separate which logic needs to go where. Since we've opted that hooks are for internal-use only, we can even define a separate type that serves as a contract between these two hooks, something like selectedProductData that merges the parent item data with the selected variation item data, which is returned by one hook and consumed by the other.
I see two ways we could restructure this, but wdyt first?
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.
🤔 Makes sense. Let me see how I can refactor 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.
@Mudaafi I made some changes, is this more of what you expect?
This PR does the following