Skip to content

Conversation

@stanlp1
Copy link
Contributor

@stanlp1 stanlp1 commented Feb 21, 2025

This PR does the following

  1. Allow itemFieldGetters to actually override the getters.
  2. Adds an imageBaseUrl field to a customConfigs object to enable overriding the image base url.

@stanlp1 stanlp1 requested a review from a team as a code owner February 21, 2025 07:42
Copy link
Contributor

@mocca102 mocca102 left a 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

https://github.com/Constructor-io/constructorio-ui-plp/pull/134/files#file-src-hooks-usecioplpprovider-ts-L30

@stanlp1 stanlp1 requested review from a team and mocca102 April 17, 2025 16:46
// 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 {
Copy link
Contributor

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

Copy link
Contributor

@Mudaafi Mudaafi Apr 22, 2025

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.

Copy link
Contributor

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 data object to generates a swatchList to render swatches
  • provides the function selectVariation to toggle which swatch is selected
  • returns the reactive selectedVariation

useProductInfo

  • Extracts fields from the Customer's data object and returns a set of fields that are relevant for the Product Card to render
  • Also calls useProductSwatch and 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 selected
  • useProductInfo - the hook in charge of parsing the Customer's unique data schema 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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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?

@stanlp1 stanlp1 requested review from Mudaafi and mocca102 May 16, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants