Refactor product list path helper #2117
Conversation
Now that this function accepts two string arguments, it would be easy to accidentally mix them up. Converting the arguments to an object will clear up this confusion.
The productListPath helper already handles itemType, so let it do it's job instead of computing it ourselves.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 Next.js Bundle Analysis for @ifixit/commerce-frontendThis analysis was generated by the Next.js Bundle Analysis action. 🤖 This PR introduced no changes to the JavaScript bundle! 🙌 |
|
QA 💠 - It seems like the product list links are still working normally and the item type canonical urls in the deploy_block 🦖 - |
According to this issue, they should canonical to the device handle by default, and the variant handles if
That sounds like the right behavior, since this setting is new and most/all product lists in Strapi have it set to false by default. |
|
un_deploy_block 👍🏻 |
| itemType: itemType ?? undefined, | ||
| variant: productList.indexVariantsInsteadOfDevice ? variant : undefined, | ||
| })}${page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : ''}`; |
There was a problem hiding this comment.
I suspect that the increased complexity of productListPath function can (at least in part) be pushed into the product list model
itemType: itemType ?? undefined,
itemType could be integrated into the product list model as an optional/nullable attribute. We already have useItemTypeProductList taking care of integrating item type override directly into the model (note that this happens to be a hook just because using Algolia hooks forces us to model it this way, otherwise that logic could be moved in the model).
variant: productList.indexVariantsInsteadOfDevice ? variant : undefined
It looks like this ternary could be implemented directly inside productListPath.
Looking at the implementation though, it looks like this kind of path that includes the variant is going to be used only in meta tags (btw, what's the reason for this behaviour?). If that's the case, we can create a specific function to compute the path and colocate it within this file, so that this logic is not made general purpose if it belongs only to a specific use case
| type ProductListPathProps = { | ||
| productList: ProductListPathAttributes; | ||
| itemType?: string; | ||
| variant?: string | undefined; | ||
| }; |
There was a problem hiding this comment.
Adding to my previous comment, it seems to me that this helper is becoming increasingly complex, and some of that complexity could be simplified if pushed into the model.
Ideally one should be able to call productListPath(productList) and get back the product list path.
itemType is just a specific type of product list, not an additional attribute I need to think about when I'm trying to get the product list path.
Maybe we can conceive a similar simplification for variant? When should I pass this param to productListPath? Can we make it so when using productListPath we don't have to think about this?
Noticed in my CR here #2112 (comment)
QA
Make sure the product list links throughout the Next.js app still work normally.
In particular, make sure that item type canonical urls in the
<head>are still correct, and check the new variant syntax too (DeviceTitle:Variant)