[CI-4169] Adds the ability to override the image url.#134
[CI-4169] Adds the ability to override the image url.#134
Conversation
mocca102
left a comment
There was a problem hiding this comment.
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.
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.
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.
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.
🤔 Makes sense. Let me see how I can refactor this.
There was a problem hiding this comment.
@Mudaafi I made some changes, is this more of what you expect?
This PR does the following