From d74b5de436882ec4f7bae2ba750e6ca4cc3c6012 Mon Sep 17 00:00:00 2001 From: Mason McElvain <52104630+masonmcelvain@users.noreply.github.com> Date: Mon, 20 Nov 2023 15:58:31 -0800 Subject: [PATCH 1/2] refactor(product list): props to path helper 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. --- frontend/helpers/path-helpers.ts | 17 +++++++++++------ frontend/seo/product-list/hreflangs.ts | 8 ++++---- .../templates/page/sections/BrowseSection.tsx | 2 +- frontend/templates/product-list/MetaTags.tsx | 10 ++++++---- .../hooks/useProductListBreadcrumbs.ts | 4 ++-- .../sections/FeaturedProductListsSection.tsx | 2 +- .../FilterableProductsSection/index.tsx | 2 +- .../sections/ProductListChildrenSection.tsx | 8 +++++--- 8 files changed, 31 insertions(+), 22 deletions(-) diff --git a/frontend/helpers/path-helpers.ts b/frontend/helpers/path-helpers.ts index 8ad98483..4ecffad4 100644 --- a/frontend/helpers/path-helpers.ts +++ b/frontend/helpers/path-helpers.ts @@ -19,12 +19,17 @@ type ProductListPathAttributes = Pick< ProductList, 'type' | 'handle' | 'deviceTitle' >; - -export function productListPath( - productList: ProductListPathAttributes, - itemType?: string, - variant?: string | undefined -): string { +type ProductListPathProps = { + productList: ProductListPathAttributes; + itemType?: string; + variant?: string | undefined; +}; + +export function productListPath({ + productList, + itemType, + variant, +}: ProductListPathProps): string { switch (productList.type) { case ProductListType.AllParts: { return '/Parts'; diff --git a/frontend/seo/product-list/hreflangs.ts b/frontend/seo/product-list/hreflangs.ts index 9ef87286..850a48d0 100644 --- a/frontend/seo/product-list/hreflangs.ts +++ b/frontend/seo/product-list/hreflangs.ts @@ -221,10 +221,10 @@ export function useHreflangs( productList: ProductList, itemType: string | null ): Hreflangs | null { - const path = productListPath(productList, itemType ?? undefined).replace( - /^\//, - '' - ); + const path = productListPath({ + productList, + itemType: itemType ?? undefined, + }).replace(/^\//, ''); const collection = pathToCollection[path] ?? null; return collection ? hreflangs[collection] : null; } diff --git a/frontend/templates/page/sections/BrowseSection.tsx b/frontend/templates/page/sections/BrowseSection.tsx index fa096385..1941f14c 100644 --- a/frontend/templates/page/sections/BrowseSection.tsx +++ b/frontend/templates/page/sections/BrowseSection.tsx @@ -157,7 +157,7 @@ function FeaturedCategories({ categories }: FeaturedCategoriesProps) { }} > diff --git a/frontend/templates/product-list/MetaTags.tsx b/frontend/templates/product-list/MetaTags.tsx index cf9d8ac8..b79ec504 100644 --- a/frontend/templates/product-list/MetaTags.tsx +++ b/frontend/templates/product-list/MetaTags.tsx @@ -123,11 +123,13 @@ function useCanonicalUrl( : ''; const variant = useVariant(); - return `${appContext.ifixitOrigin}${productListPath( + return `${appContext.ifixitOrigin}${productListPath({ productList, - undefined, - productList.indexVariantsInsteadOfDevice ? variant : undefined - )}${itemTypeHandle}${page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : ''}`; + itemType: undefined, + variant: productList.indexVariantsInsteadOfDevice ? variant : undefined, + })}${itemTypeHandle}${ + page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : '' + }`; } function useShouldNoIndex( diff --git a/frontend/templates/product-list/hooks/useProductListBreadcrumbs.ts b/frontend/templates/product-list/hooks/useProductListBreadcrumbs.ts index d54c6bd9..db972ebb 100644 --- a/frontend/templates/product-list/hooks/useProductListBreadcrumbs.ts +++ b/frontend/templates/product-list/hooks/useProductListBreadcrumbs.ts @@ -16,7 +16,7 @@ export function useProductListBreadcrumbs( productList.ancestors.forEach((ancestor) => { breadcrumbs.push({ label: ancestor.title, - url: productListPath(ancestor), + url: productListPath({ productList: ancestor }), }); }); @@ -27,7 +27,7 @@ export function useProductListBreadcrumbs( if (productList.type === ProductListType.DeviceParts && itemType) { breadcrumbs.push({ label: productList.title, - url: productListPath(productList), + url: productListPath({ productList }), }); breadcrumbs.push({ label: itemType, diff --git a/frontend/templates/product-list/sections/FeaturedProductListsSection.tsx b/frontend/templates/product-list/sections/FeaturedProductListsSection.tsx index 5a2b3482..42217f30 100644 --- a/frontend/templates/product-list/sections/FeaturedProductListsSection.tsx +++ b/frontend/templates/product-list/sections/FeaturedProductListsSection.tsx @@ -88,7 +88,7 @@ const ProductListLink = ({ productList }: ProductListLinkProps) => { )} diff --git a/frontend/templates/product-list/sections/FilterableProductsSection/index.tsx b/frontend/templates/product-list/sections/FilterableProductsSection/index.tsx index eb301d18..453817d4 100644 --- a/frontend/templates/product-list/sections/FilterableProductsSection/index.tsx +++ b/frontend/templates/product-list/sections/FilterableProductsSection/index.tsx @@ -349,7 +349,7 @@ const ProductListEmptyState = forwardRef( This collection does not have products. {parentCategory && ( - + Return to {parentCategory.title} )} diff --git a/frontend/templates/product-list/sections/ProductListChildrenSection.tsx b/frontend/templates/product-list/sections/ProductListChildrenSection.tsx index 01abdde3..3f535c52 100644 --- a/frontend/templates/product-list/sections/ProductListChildrenSection.tsx +++ b/frontend/templates/product-list/sections/ProductListChildrenSection.tsx @@ -68,9 +68,11 @@ export function ProductListChildrenSection({ Date: Mon, 20 Nov 2023 16:01:10 -0800 Subject: [PATCH 2/2] refactor(product list): dedupe canonical item type The productListPath helper already handles itemType, so let it do it's job instead of computing it ourselves. --- frontend/templates/product-list/MetaTags.tsx | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/frontend/templates/product-list/MetaTags.tsx b/frontend/templates/product-list/MetaTags.tsx index b79ec504..bb8ac560 100644 --- a/frontend/templates/product-list/MetaTags.tsx +++ b/frontend/templates/product-list/MetaTags.tsx @@ -1,7 +1,6 @@ import { PRODUCT_LIST_PAGE_PARAM } from '@config/constants'; import { metaTitleWithSuffix } from '@helpers/metadata-helpers'; import { productListPath } from '@helpers/path-helpers'; -import { stylizeDeviceItemType } from '@helpers/product-list-helpers'; import { useAppContext } from '@ifixit/app'; import type { ProductList } from '@models/product-list'; import { noIndexExemptions, useHreflangs } from '@seo/product-list'; @@ -115,21 +114,14 @@ function useCanonicalUrl( ): string { const appContext = useAppContext(); const pagination = usePagination(); - - const page = pagination.currentRefinement + 1; - - const itemTypeHandle = itemType - ? `/${encodeURIComponent(stylizeDeviceItemType(itemType))}` - : ''; const variant = useVariant(); + const page = pagination.currentRefinement + 1; return `${appContext.ifixitOrigin}${productListPath({ productList, - itemType: undefined, + itemType: itemType ?? undefined, variant: productList.indexVariantsInsteadOfDevice ? variant : undefined, - })}${itemTypeHandle}${ - page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : '' - }`; + })}${page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : ''}`; } function useShouldNoIndex(