Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor product list path helper #2117

Merged
merged 2 commits into from
Nov 21, 2023
Merged

Refactor product list path helper #2117

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d74b5de
refactor(product list): props to path helper
masonmcelvain Nov 20, 2023
ced2146
refactor(product list): dedupe canonical item type
masonmcelvain Nov 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions frontend/helpers/path-helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
};
Comment on lines +22 to +26
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


export function productListPath({
productList,
itemType,
variant,
}: ProductListPathProps): string {
switch (productList.type) {
case ProductListType.AllParts: {
return '/Parts';
Expand Down
8 changes: 4 additions & 4 deletions frontend/seo/product-list/hreflangs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
2 changes: 1 addition & 1 deletion frontend/templates/page/sections/BrowseSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ function FeaturedCategories({ categories }: FeaturedCategoriesProps) {
}}
>
<NextLink
href={productListPath(category)}
href={productListPath({ productList: category })}
passHref
legacyBehavior
>
Expand Down
16 changes: 5 additions & 11 deletions frontend/templates/product-list/MetaTags.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -115,19 +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(
return `${appContext.ifixitOrigin}${productListPath({
productList,
undefined,
productList.indexVariantsInsteadOfDevice ? variant : undefined
)}${itemTypeHandle}${page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : ''}`;
itemType: itemType ?? undefined,
variant: productList.indexVariantsInsteadOfDevice ? variant : undefined,
})}${page > 1 ? `?${PRODUCT_LIST_PAGE_PARAM}=${page}` : ''}`;
Comment on lines +122 to +124
Copy link
Contributor

@dhmacs dhmacs Nov 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

function useShouldNoIndex(
Expand Down
4 changes: 2 additions & 2 deletions frontend/templates/product-list/hooks/useProductListBreadcrumbs.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ export function useProductListBreadcrumbs(
productList.ancestors.forEach((ancestor) => {
breadcrumbs.push({
label: ancestor.title,
url: productListPath(ancestor),
url: productListPath({ productList: ancestor }),
});
});

Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion frontend/templates/product-list/sections/FeaturedProductListsSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ const ProductListLink = ({ productList }: ProductListLinkProps) => {
)}
<Divider orientation="vertical" />
<NextLink
href={productListPath(productList)}
href={productListPath({ productList })}
passHref
legacyBehavior
>
Expand Down
2 changes: 1 addition & 1 deletion frontend/templates/product-list/sections/FilterableProductsSection/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,7 +349,7 @@ const ProductListEmptyState = forwardRef<EmptyStateProps, 'div'>(
This collection does not have products.
</Text>
{parentCategory && (
<Link href={productListPath(parentCategory)}>
<Link href={productListPath({ productList: parentCategory })}>
Return to {parentCategory.title}
</Link>
)}
Expand Down
8 changes: 5 additions & 3 deletions frontend/templates/product-list/sections/ProductListChildrenSection.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,11 @@ export function ProductListChildrenSection({
<ProductListCard
as="a"
href={productListPath({
deviceTitle: child.deviceTitle,
handle: child.handle,
type: child.type,
productList: {
deviceTitle: child.deviceTitle,
handle: child.handle,
type: child.type,
},
})}
productList={{
title: child.title,
Expand Down