Improve collection filters, fix minor bugs #233
Conversation
WalkthroughThe pull request introduces several modifications across multiple files, primarily focusing on enhancing the Changes
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (12)
app/components/checkbox.tsx (1)
17-21: Refactor className forCheckboxPrimitive.RootThe class names are now being managed using the
cnutility function, which improves readability and maintainability by organizing classes across multiple lines.app/lib/filter.ts (1)
52-57: Document the price filter behaviorThe special handling for price filters (overwriting instead of appending) should be documented to explain why this behavior differs from other filters.
if (k === "price") { - // For price, we want to overwrite + // Price range should be exclusive - overwrite existing values + // instead of appending to prevent multiple conflicting ranges params.set(key, value); } else { params.append(key, value); }app/sections/collection-filters/sort.tsx (1)
41-45: Enhance accessibility for sort triggerThe sort trigger button should have an ARIA label to improve screen reader experience.
- <DropdownMenu.Trigger className="flex items-center gap-1.5 h-12 border px-4 py-2.5 focus-visible:outline-none"> + <DropdownMenu.Trigger + className="flex items-center gap-1.5 h-12 border px-4 py-2.5 focus-visible:outline-none" + aria-label={`Sort by ${currentSort.label}`} + >app/sections/collection-filters/tools-bar.tsx (1)
33-33: Consider using Tailwind's border utilities consistently.The border classes could be combined using Tailwind's border utility class.
-<div className="border-y border-line-subtle py-4"> +<div className="border-y-[1px] border-line-subtle py-4">app/styles/app.css (1)
51-61: LGTM! Consider adding focus styles for better accessibility.The styles effectively normalize number input appearance across browsers, which is crucial for price filter inputs. However, consider enhancing accessibility:
Add focus styles to improve keyboard navigation:
input[type="number"] { -webkit-appearance: textfield; -moz-appearance: textfield; appearance: textfield; + &:focus { + @apply outline-none ring-2 ring-primary ring-offset-1; + } }app/sections/collection-filters/filter-item.tsx (2)
30-32: Consider using strict equality comparison for filter matchingThe current implementation uses
JSON.stringifyfor object comparison which might be fragile if the order of properties changes.Consider using a more robust comparison:
- let filter = appliedFilters.find( - (filter) => JSON.stringify(filter.filter) === option.input, - ); + let filter = appliedFilters.find((filter) => { + const inputFilter = JSON.parse(option.input as string); + return Object.entries(filter.filter).every( + ([key, value]) => inputFilter[key] === value + ); + });
47-88: Consider extracting color swatch rendering logicThe color swatch rendering logic is complex and could benefit from being extracted into a separate component for better maintainability.
Consider creating a
ColorSwatchFiltercomponent:function ColorSwatchFilter({ option, checked, handleCheckedChange, swatches, options, showFiltersCount }: ColorSwatchFilterProps) { let swatchColor = swatches.colors.find(({ name }) => name === option.label); let optionConf = options.find(({ name }) => name.toLowerCase() === option.label.toLowerCase() ); let { shape = "square", size = "md" } = optionConf || {}; return ( <Tooltip> <TooltipTrigger> {/* ... existing button code ... */} </TooltipTrigger> <TooltipContent> <FilterLabel option={option} showFiltersCount={showFiltersCount} /> </TooltipContent> </Tooltip> ); }app/sections/collection-filters/products-pagination.tsx (1)
117-121: Enhance empty state messagingThe current empty state message could be more helpful by suggesting actions users can take.
<div className="gap-3 pt-20 flex justify-center items-center flex-col"> <FunnelX size={50} weight="light" /> - <div className="text-lg">No products matched your filters.</div> + <div className="text-lg text-center"> + <p>No products matched your filters.</p> + <p className="text-sm text-body-subtle mt-2"> + Try adjusting your filters or clearing them to see more products. + </p> + </div> </div>app/routes/($locale).collections.$collectionHandle.tsx (2)
Line range hint
41-67: Consider adding error boundary for filter parsingThe filter parsing logic could throw if invalid JSON is provided in the URL.
let filters = [...searchParams.entries()].reduce((filters, [key, value]) => { if (key.startsWith(FILTER_URL_PREFIX)) { let filterKey = key.substring(FILTER_URL_PREFIX.length); + try { filters.push({ [filterKey]: JSON.parse(value), }); + } catch (e) { + console.error(`Invalid filter value for ${filterKey}:`, value); + } } return filters; }, [] as ProductFilter[]);
Line range hint
89-108: Consider caching filter values computationThe
allFilterValuescomputation and subsequent filter matching could be expensive for large collections.Consider memoizing the filter values:
const memoizedFilterValues = useMemo( () => collection.products.filters.flatMap(filter => filter.values), [collection.products.filters] );app/sections/collection-filters/price-range-filter.tsx (2)
41-53: Add input validation in handleFilterConsider validating that maxPrice is greater than minPrice before updating the URL parameters.
function handleFilter() { if (minPrice === undefined && maxPrice === undefined) { params.delete(`${FILTER_URL_PREFIX}price`); navigate(`${location.pathname}?${params.toString()}`); return; } + if (minPrice !== undefined && maxPrice !== undefined && minPrice > maxPrice) { + return; + } let price = { ...(minPrice === undefined ? {} : { min: minPrice }), ...(maxPrice === undefined ? {} : { max: maxPrice }), }; let newParams = filterInputToParams({ price }, params); navigate(`${location.pathname}?${newParams.toString()}`); }
105-120: Enhance input field UXConsider adding these improvements to the price input fields:
- Add step attribute to prevent decimal inputs if not needed
- Add aria-label for currency symbol
- Format the placeholder value for better readability
<input name="minPrice" type="number" + step="1" value={minPrice ?? ""} min={minVariantPrice.amount} - placeholder={Number(minVariantPrice.amount).toString()} + placeholder={new Intl.NumberFormat().format(Number(minVariantPrice.amount))} onChange={(e) => {Also applies to: 130-145
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
app/components/checkbox.tsx(1 hunks)app/data/cache.ts(1 hunks)app/data/fragments.ts(1 hunks)app/data/queries.ts(1 hunks)app/hooks/use-closest-weaverse-item.ts(1 hunks)app/lib/collections.ts(1 hunks)app/lib/filter.ts(1 hunks)app/routes/($locale).collections.$collectionHandle.tsx(1 hunks)app/sections/collection-filters/filter-item.tsx(1 hunks)app/sections/collection-filters/filters.tsx(3 hunks)app/sections/collection-filters/price-range-filter.tsx(1 hunks)app/sections/collection-filters/products-pagination.tsx(3 hunks)app/sections/collection-filters/sort.tsx(3 hunks)app/sections/collection-filters/tools-bar.tsx(3 hunks)app/sections/product-list.tsx(1 hunks)app/styles/app.css(1 hunks)app/weaverse/components.ts(1 hunks)package.json(2 hunks)storefrontapi.generated.d.ts(14 hunks)vite.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- app/data/cache.ts
- app/hooks/use-closest-weaverse-item.ts
🔇 Additional comments (16)
app/sections/collection-filters/filters.tsx (3)
2-2: Import statement for useLoaderData
The import of useLoaderData from @remix-run/react is appropriate and necessary for accessing data loaded by the route.
8-8: Import of cn utility function
The cn function is a common utility for class name manipulation and its import from "~/lib/cn" is appropriate.
11-12: Importing new components FilterItem and PriceRangeFilter
The addition of these imports aligns with the usage of these components within the file.
storefrontapi.generated.d.ts (1)
47-50: Updates to include priceRange in product fragments
The additions of the priceRange fields to various product fragments enhance the data available for products, allowing for richer pricing information throughout the application. This change is consistent across multiple query types and seems appropriately implemented.
Also applies to: 551-560, 593-602, 696-705, 736-767, 834-843, 1005-1014, 1414-1423, 1738-1747, 1821-1829, 1837-1849, 1865-1873, 1893-1901
app/components/checkbox.tsx (1)
25-25: Update Checkbox indicator to use a styled span
Replacing the Check icon with a span element styled as bg-body changes the visual representation of the checkbox's checked state. Ensure this aligns with the design requirements of the application.
If the intention is to have a custom indicator, consider whether additional accessibility attributes or considerations are necessary.
app/lib/collections.ts (1)
4-42: Function getSortValuesFromParam correctly maps sort parameters
The newly introduced getSortValuesFromParam function accurately maps the SortParam values to the corresponding ProductCollectionSortKeys and reverse flags required by the Shopify API.
vite.config.ts (1)
18-18: Verify the impact of disabling v3_singleFetch on filter performance
Disabling the v3_singleFetch optimization might affect the performance of collection filters, especially when multiple data fetches are involved.
app/sections/collection-filters/sort.tsx (1)
Line range hint 8-29: LGTM! Clear and consistent sort options
The sort options are well-organized with clear labels and consistent formatting. The addition of the relevance option and the improved price sort labels enhance user understanding.
package.json (1)
5-5: LGTM! Version bump and new dependency addition look good.
The version bump follows semantic versioning, and @radix-ui/react-slider is a solid choice for implementing accessible range inputs.
Also applies to: 35-35
app/sections/product-list.tsx (1)
9-9: Verify the getSortValuesFromParam function exists in the new location.
Moving the sorting logic to ~/lib/collections improves reusability. Let's verify the function exists in its new location.
✅ Verification successful
Function successfully moved to the new location with proper implementation
The getSortValuesFromParam function exists in app/lib/collections.ts with proper type imports and implementation for handling different sort parameters. The function is well-structured and maintains its sorting logic functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify getSortValuesFromParam function exists in lib/collections
# and check its implementation
# Check if the function exists in the new location
rg -l "getSortValuesFromParam" "app/lib/collections.ts"
# Show the implementation if found
rg -A 10 "export.*getSortValuesFromParam" "app/lib/collections.ts"
Length of output: 396
Script:
#!/bin/bash
# Get more context around the function implementation
rg -A 20 'export let getSortValuesFromParam' "app/lib/collections.ts"
# Check for any imports or type definitions needed by the function
rg -B 10 'export let getSortValuesFromParam' "app/lib/collections.ts"
Length of output: 783
app/sections/collection-filters/tools-bar.tsx (2)
Line range hint 13-20: LGTM! Props interface improvements.
The simplified interface with explicit filter configuration props improves the component's API and makes it more maintainable.
42-42: Consider maintaining text case consistency.
The change to lowercase "products" should be verified against the design system's text casing guidelines.
✅ Verification successful
Text casing appears to be consistent with existing usage
Based on the search results, the lowercase "products" is consistent with other instances in the codebase, particularly in similar contexts like search results description showing "search results" and product count checks. The current implementation follows the established pattern.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check text casing consistency across the codebase
# Look for similar count/total labels to verify casing convention
rg -i "\d+\s+(products?|items?)" --type tsx
Length of output: 73
Script:
#!/bin/bash
# Description: Check text casing consistency across the codebase
# Look for similar count/total labels to verify casing convention
# Using different file extensions as tsx wasn't recognized
rg -i "\d+\s+(products?|items?)" --type ts --type js --type jsx
Length of output: 93
Script:
#!/bin/bash
# Description: Check text casing consistency across the codebase
# Look for similar count/total labels to verify casing convention
# Using ripgrep without file type filters, then filter results manually
rg -i "\d+\s+(products?|items?)"
Length of output: 1997
Script:
#!/bin/bash
# Let's try a more specific search to find product count labels
rg -B2 -A2 "products?.nodes?.length"
Length of output: 1198
app/data/fragments.ts (1)
47-56: LGTM! The price range addition enhances product filtering capabilities.
The new priceRange field with minVariantPrice and maxVariantPrice is well-structured and provides essential data for implementing price-based filtering features.
Let's verify the usage of this fragment:
✅ Verification successful
The priceRange addition is properly integrated and utilized
The verification shows that:
- The
priceRangefield is actively used in the price range filter component (price-range-filter.tsx) to implement min/max price filtering functionality - The
PRODUCT_CARD_FRAGMENTis correctly imported and used across multiple queries in the application - The implementation properly handles the new price range data structure with appropriate null checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check where PRODUCT_CARD_FRAGMENT is used and if the new priceRange field is being accessed
# Find files using PRODUCT_CARD_FRAGMENT
rg -l "PRODUCT_CARD_FRAGMENT"
# Check for priceRange usage in components
rg "priceRange" app/components app/sections
Length of output: 504
Script:
#!/bin/bash
# Let's check the implementation details of files using PRODUCT_CARD_FRAGMENT
# to ensure they handle the new priceRange field correctly
# Check price-range-filter.tsx implementation
cat app/sections/collection-filters/price-range-filter.tsx
# Check how the fragment is used in routes
rg -A 5 "PRODUCT_CARD_FRAGMENT" app/routes/\(\$locale\).search.tsx app/routes/\(\$locale\).api.products.tsx app/routes/\(\$locale\).api.featured-items.ts
# Check queries.ts implementation
rg -A 5 "PRODUCT_CARD_FRAGMENT" app/data/queries.ts
Length of output: 8311
app/sections/collection-filters/filter-item.tsx (1)
76-79: 🛠️ Refactor suggestion
Add fallback color for invalid color values
The current implementation might render an invalid background color if the color value is not a valid CSS color.
Let's verify the color values in use:
Add a fallback color:
style={{
backgroundColor:
- swatchColor?.value || option.label.toLowerCase(),
+ swatchColor?.value ||
+ (isValidCSSColor(option.label.toLowerCase()) ? option.label.toLowerCase() : '#e5e5e5'),
}}app/sections/collection-filters/price-range-filter.tsx (1)
1-10: Well-structured imports with good accessibility considerations!
Good choice of Radix UI components for building an accessible price range filter.
app/data/queries.ts (1)
210-243: Well-structured query additions for price range data
The new fields efficiently fetch only the necessary price data for the filter. Good use of the sortKey: PRICE parameter to get the price boundaries.
Let's verify that these fields are being used correctly throughout the codebase:
✅ Verification successful
Price range filter implementation verified and properly integrated
The verification shows that:
- The new fields are correctly typed in
storefrontapi.generated.d.ts - The price range filter component properly destructures and uses
highestPriceProductandlowestPriceProductto set the min/max price boundaries - The implementation safely handles null values through optional chaining
- The price filter correctly uses the data for both default values and user-selected price ranges
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper usage of the new price-related fields
# Look for references to highestPriceProduct and lowestPriceProduct
rg -l "highestPriceProduct|lowestPriceProduct"
# Check if the COLLECTION_QUERY is imported where price range filter is used
rg -l "COLLECTION_QUERY.*price-range-filter"
Length of output: 199
Script:
#!/bin/bash
# Let's check how these fields are used in the price range filter component
rg -A 10 "highestPriceProduct|lowestPriceProduct" "app/sections/collection-filters/price-range-filter.tsx"
# Also check the type definitions to ensure proper typing
rg -A 5 "highestPriceProduct|lowestPriceProduct" "storefrontapi.generated.d.ts"
# Let's verify the COLLECTION_QUERY import in the price filter component
rg "COLLECTION_QUERY" "app/sections/collection-filters/price-range-filter.tsx"
Length of output: 5019
| <PriceRangeFilter | ||
| key={option.id} | ||
| collection={ | ||
| collection as CollectionDetailsQuery["collection"] | ||
| } | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid rendering PriceRangeFilter multiple times
Currently, the PriceRangeFilter component is being rendered within the .map() function over filter.values, which could result in multiple instances of PriceRangeFilter being rendered unnecessarily.
Since price range is a single filter and doesn't depend on multiple option values, you should render PriceRangeFilter once per price filter, not once per filter value.
Apply this diff to render PriceRangeFilter outside of the filter.values.map():
{filters.map((filter: Filter) => {
let asColorSwatch =
enableColorSwatch && COLORS_FILTERS.includes(filter.label);
let asButton = displayAsButtonFor.includes(filter.label);
return (
<Accordion.Item
key={filter.id}
value={filter.id}
className="w-full pb-6 pt-7"
>
<Accordion.Trigger className="flex w-full justify-between items-center [&>svg]:data-[state=open]:rotate-90">
<span>{filter.label}</span>
<IconCaretRight className="w-4 h-4 transition-transform rotate-0" />
</Accordion.Trigger>
<Accordion.Content
style={
{
"--slide-up-from": "var(--radix-accordion-content-height)",
"--slide-down-to": "var(--radix-accordion-content-height)",
"--slide-up-duration": "0.15s",
"--slide-down-duration": "0.15s",
} as React.CSSProperties
}
className={clsx([
"overflow-hidden",
"data-[state=closed]:animate-slide-up",
"data-[state=open]:animate-slide-down",
])}
>
<div
className={clsx(
"flex pt-8",
asColorSwatch || asButton
? "gap-1.5 flex-wrap"
: "flex-col gap-5",
)}
>
+ {filter.type === "PRICE_RANGE" ? (
+ <PriceRangeFilter
+ key={filter.id}
+ collection={
+ collection as CollectionDetailsQuery["collection"]
+ }
+ />
+ ) : (
{filter.values?.map((option) => (
<FilterItem
key={option.id}
displayAs={
asColorSwatch
? "color-swatch"
: asButton
? "button"
: "list-item"
}
appliedFilters={appliedFilters as AppliedFilter[]}
option={option}
showFiltersCount={showFiltersCount}
/>
))}
+ )}
</div>
</Accordion.Content>
</Accordion.Item>
);
})}This change ensures that PriceRangeFilter is rendered once per price filter and avoids unnecessary duplication.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <PriceRangeFilter | |
| key={option.id} | |
| collection={ | |
| collection as CollectionDetailsQuery["collection"] | |
| } | |
| /> | |
| {filter.type === "PRICE_RANGE" ? ( | |
| <PriceRangeFilter | |
| key={filter.id} | |
| collection={ | |
| collection as CollectionDetailsQuery["collection"] | |
| } | |
| /> | |
| ) : ( | |
| filter.values?.map((option) => ( | |
| <FilterItem | |
| key={option.id} | |
| displayAs={ | |
| asColorSwatch | |
| ? "color-swatch" | |
| : asButton | |
| ? "button" | |
| : "list-item" | |
| } | |
| appliedFilters={appliedFilters as AppliedFilter[]} | |
| option={option} | |
| showFiltersCount={showFiltersCount} | |
| /> | |
| )) | |
| )} |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/sections/collection-filters/filters.tsx (1)
83-105: Consider improving type safety and readabilityA few suggestions to enhance the code:
- The type assertion for collection prop seems unnecessary
- The nested ternary in displayAs prop could be simplified
Consider these improvements:
{filter.type === "PRICE_RANGE" ? ( <PriceRangeFilter - collection={ - collection as CollectionDetailsQuery["collection"] - } + collection={collection} /> ) : ( filter.values?.map((option) => ( <FilterItem key={option.id} - displayAs={ - asColorSwatch - ? "color-swatch" - : asButton - ? "button" - : "list-item" - } + displayAs={asColorSwatch ? "color-swatch" : asButton ? "button" : "list-item"} appliedFilters={appliedFilters as AppliedFilter[]} option={option} showFiltersCount={showFiltersCount} /> )) )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/sections/collection-filters/filters.tsx(3 hunks)
🔇 Additional comments (2)
app/sections/collection-filters/filters.tsx (2)
40-42: Reconsider the use of concatenated key prop
Using a concatenated string of multiple variables (collection.id + appliedFiltersKeys + expandFilters + showFiltersCount) as a key prop could cause unnecessary re-mounting of the Accordion component.
Consider using component state or effects to manage Accordion's behavior instead.
83-89: LGTM! PriceRangeFilter implementation is improved
The conditional rendering of PriceRangeFilter addresses the previous concern about multiple renders. This is a good improvement.
hta218
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
app/components/header/cart-count.tsx (1)
56-60: Consider accessibility improvements for cart badge.While updating the text size from
text-[12px]totext-sm, ensure the contrast ratio meets WCAG standards, especially with the transparent header variations.Consider adding these accessibility improvements:
- "text-sm leading-none text-center font-medium subpixel-antialiased", + "text-sm leading-none text-center font-medium subpixel-antialiased min-h-[20px]", + "aria-label={`Cart items: ${count}`}",app/components/header/mobile-header.tsx (1)
64-64: Consider adding loading state for CartDrawer.The CartDrawer is rendered without a loading fallback, which might cause layout shifts during initial load.
Consider wrapping the CartDrawer with Suspense:
- <CartDrawer isTransparent={isTransparent} /> + <Suspense fallback={<div className="w-8 h-8" />}> + <CartDrawer isTransparent={isTransparent} /> + </Suspense>app/components/header/cart-drawer.tsx (2)
62-62: Consider adding an aria-describedby attribute.The Dialog.Content component has aria-describedby explicitly set to undefined. Consider adding a description for better accessibility.
-aria-describedby={undefined} +aria-describedby="cart-content-description"Also, add a hidden description:
<div id="cart-content-description" className="sr-only"> View your shopping cart and proceed to checkout </div>
78-78: Consider dynamic height calculation.The max-height calculation is hardcoded. Consider using a CSS custom property for better maintainability.
-className="max-h-[calc(100vh-4.5rem)]" +style={{ maxHeight: 'calc(100vh - var(--header-height, 4.5rem))' }}app/modules/cart.tsx (2)
160-166: Consider maintaining semantic HTML structure.The container was changed from
<section>to<div>. While this works, semantic HTML helps with accessibility and document structure.-<div +<section ref={scrollRef} aria-labelledby="cart-contents" className={clsx([ y > 0 ? "border-t border-line-subtle" : "", layout === "page" && "flex-grow md:translate-y-4 lg:col-span-2", layout === "drawer" && "overflow-auto transition", ])} >
215-218: Consider consistent border styling approach.The border styling is mixed between utility classes and custom properties. Consider using a consistent approach for better maintainability.
-"grid gap-4 border-t border-line-subtle pt-4" +"grid gap-4 border-t border-[--color-line-subtle] pt-4"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/components/drawer.tsx(0 hunks)app/components/header/cart-count.tsx(2 hunks)app/components/header/cart-drawer.tsx(1 hunks)app/components/header/desktop-header.tsx(2 hunks)app/components/header/index.tsx(2 hunks)app/components/header/menu/mobile-menu.tsx(1 hunks)app/components/header/mobile-header.tsx(2 hunks)app/modules/cart.tsx(5 hunks)app/sections/collection-filters/products-pagination.tsx(3 hunks)app/sections/collection-filters/tools-bar.tsx(4 hunks)
💤 Files with no reviewable changes (1)
- app/components/drawer.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- app/sections/collection-filters/products-pagination.tsx
🔇 Additional comments (9)
app/sections/collection-filters/tools-bar.tsx (2)
Line range hint 13-20: LGTM! Interface simplification improves maintainability.
The removal of width-based variants in favor of simpler props structure is a good improvement. The interface remains type-safe and self-documenting.
Line range hint 33-42: Verify the product count text style requirements.
The product count text has been changed to lowercase ("products" instead of "Products"). Please verify if this aligns with the design system's text casing guidelines.
✅ Verification successful
Product count text casing appears to be consistent with the codebase
The search results show that the lowercase "products" usage in the count display is consistent with other instances in the codebase:
- Used as lowercase in search results description
- Used as lowercase in product list configuration
- Used as lowercase in cart best sellers documentation
The current implementation follows the established pattern across the codebase, so no changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of similar count labels to ensure consistency
rg -i '\d+\s+(products?|items?|results?)' --type tsx --type jsx
Length of output: 93
Script:
#!/bin/bash
# Search for other instances of similar count labels to ensure consistency
# Using ripgrep without file type restrictions and with context
rg -i '\d+\s+(products?|items?|results?)' -C 2
Length of output: 7444
Script:
#!/bin/bash
# Let's search specifically for product count displays in the codebase
rg -i 'products?.nodes?.length|number of products?' -C 2
Length of output: 1950
app/components/header/menu/mobile-menu.tsx (1)
41-43: Verify accessibility after removing VisuallyHidden wrapper
The removal of VisuallyHidden.Root changes how screen readers interpret the menu title. While the implementation is simpler, we should ensure it maintains accessibility standards.
Let's check for other accessibility patterns in the codebase:
Consider adding an aria-label for better screen reader context:
- <Dialog.Title asChild>
- <div className="px-4">Menu</div>
- </Dialog.Title>
+ <Dialog.Title asChild>
+ <div className="px-4" aria-label="Navigation Menu">Menu</div>
+ </Dialog.Title>✅ Verification successful
Removing VisuallyHidden is acceptable in this context
Based on the codebase analysis, the removal of VisuallyHidden is appropriate here because:
- The menu title "Menu" is now visibly displayed, serving both visual and screen reader users
- The Dialog component from Radix UI maintains proper accessibility semantics with Dialog.Title
- Other similar Dialog implementations in the codebase (tools-bar.tsx, cart-drawer.tsx) also use visible titles without VisuallyHidden
The current implementation follows consistent patterns across the codebase and maintains accessibility standards through Radix UI's semantic structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for accessibility patterns and VisuallyHidden usage
# Look for similar Dialog.Title implementations and VisuallyHidden usage
# Check for Dialog.Title implementations
ast-grep --pattern 'Dialog.Title {
$$$
}'
# Check for VisuallyHidden usage
rg -l "VisuallyHidden"
# Check for aria-label usage with Dialog
rg "Dialog.*aria-label"
Length of output: 199
Script:
#!/bin/bash
# Let's check the Dialog usage patterns more thoroughly
# Check Dialog.Title patterns with more context
rg "Dialog.Title" -A 3 -B 3
# Check how VisuallyHidden is used in the found files
cat app/sections/collection-filters/price-range-filter.tsx app/components/header/predictive-search/index.tsx
# Check for other accessibility patterns in mobile-menu.tsx
cat app/components/header/menu/mobile-menu.tsx
# Check for Dialog accessibility patterns in the codebase
rg "Dialog.*" -A 2 -B 2 --type tsx
Length of output: 14164
app/components/header/index.tsx (2)
1-2: LGTM! Clean up of imports improves code organization.
The removal of unused imports and moving cart-related functionality to a dedicated component follows good separation of concerns.
Also applies to: 4-6
10-11: Verify cart drawer state management across components.
The cart drawer state management has been simplified to use the useDrawer hook. Please ensure this change maintains the expected behavior across all user interactions.
✅ Verification successful
Let me gather more information about the cart drawer implementation to verify the state management.
Cart drawer state management is correctly implemented and consistent
The verification shows that the cart drawer state management is properly implemented:
- The
useDrawerhook is only used in the header component for cart functionality - The cart drawer state is properly handled in
app/components/header/index.tsxwith correct effect dependencies - The implementation is consistent across desktop and mobile header components
- The drawer state is correctly integrated with cart actions through the
useCartFetchershook
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all useDrawer hook usages to verify consistent state management
rg -A 3 "useDrawer\(" app/
Length of output: 644
Script:
#!/bin/bash
# Search for cart drawer related components and state management
rg -A 5 "cart.*drawer" app/ -i
# Look for any other drawer components that might be affected
ast-grep --pattern 'function $name() {
$$$
useDrawer($$$)
$$$
}'
# Check for any drawer state dependencies
rg "isOpen|openDrawer" app/
Length of output: 4683
app/components/header/mobile-header.tsx (2)
Line range hint 1-1: Verify PR objectives alignment.
The changes focus on cart functionality improvements and bug fixes, but the PR title mentions "Improve collection filters" which isn't reflected in these changes. Please clarify if there are additional changes related to collection filters or update the PR title accordingly.
16-16: Verify CartDrawer component integration.
The CartDrawer component has been introduced to replace the previous cart functionality. Please ensure:
- The drawer behavior is consistent across mobile devices
- The transition animations work smoothly
- The drawer respects the viewport boundaries
Also applies to: 19-19
✅ Verification successful
Let me gather more information about the CartDrawer implementation and its styling to verify the integration concerns.
Let me check the animation definitions and styling utilities that handle the drawer transitions.
Let me check the tailwind configuration for animations and the root configuration files.
Based on the comprehensive analysis of the CartDrawer implementation and animations, I can now provide the final response:
CartDrawer integration is properly implemented with appropriate transitions and viewport handling
The CartDrawer component is well-integrated with:
- Proper viewport boundaries using fixed positioning and max-width
- Smooth transitions using tailwind animations:
- Fade-in overlay animation with 100ms duration
- Slide-in animation from right with transform transitions
- Proper z-index layering for overlay and drawer content
- Responsive design with max-width of 400px and full viewport height
- Proper mobile viewport handling with ScrollArea component
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for CartDrawer usage and related styling
rg "CartDrawer" app/
rg -A 5 "drawer.*transition|transition.*drawer" app/
Length of output: 801
Script:
#!/bin/bash
# Check CartDrawer implementation and styling
cat app/components/header/cart-drawer.tsx
# Look for drawer-related styling and animations
rg -A 3 "transition|transform|translate|animation" app/components/header/cart-drawer.tsx
rg -A 3 "@keyframes.*drawer" app/
Length of output: 4429
Script:
#!/bin/bash
# Check for animation definitions in the CSS files
rg -A 5 "animate-fade-in|animate-enter-from-right" app/
# Check for any global drawer styling
rg -A 5 "drawer" app/styles/
rg -A 5 "drawer" app/tailwind.config.*
Length of output: 6277
Script:
#!/bin/bash
# Check tailwind config and animation definitions
fd -e js -e ts "tailwind.config" .
fd -e js -e ts "config" . | grep -i tailwind
cat tailwind.config.js
# Check for any animation utilities
rg -A 5 "keyframes|animation" tailwind.config.js
Length of output: 6903
app/components/header/desktop-header.tsx (1)
75-75: LGTM! CartDrawer integration looks good.
The CartDrawer component is properly integrated with the isTransparent prop, maintaining the header's transparency behavior.
app/components/header/cart-drawer.tsx (1)
30-31: Good use of analytics tracking.
The analytics event is properly triggered when the cart is viewed, providing valuable user interaction data.
| aria-describedby={undefined} | ||
| > | ||
| <div className="space-y-1"> | ||
| <div className="flex gap-2 items-center justify-between px-4"> | ||
| <span className="py-2.5 font-bold">Filters</span> | ||
| <Dialog.Title asChild className="py-2.5 font-bold"> | ||
| <span>Filters</span> | ||
| </Dialog.Title> |
There was a problem hiding this comment.
Fix accessibility attribute implementation.
The aria-describedby={undefined} is problematic:
- If no description is needed, remove the attribute entirely
- If a description is needed, add a description element and reference its ID
- aria-describedby={undefined}Additionally, consider adding a description of the filters functionality for screen readers:
+ <div id="filters-description" className="sr-only">
+ Filter your product results by various criteria
+ </div>
<div className="space-y-1">Then reference it:
- aria-describedby={undefined}
+ aria-describedby="filters-description"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| aria-describedby={undefined} | |
| > | |
| <div className="space-y-1"> | |
| <div className="flex gap-2 items-center justify-between px-4"> | |
| <span className="py-2.5 font-bold">Filters</span> | |
| <Dialog.Title asChild className="py-2.5 font-bold"> | |
| <span>Filters</span> | |
| </Dialog.Title> | |
| aria-describedby="filters-description" | |
| > | |
| <div id="filters-description" className="sr-only"> | |
| Filter your product results by various criteria | |
| </div> | |
| <div className="space-y-1"> | |
| <div className="flex gap-2 items-center justify-between px-4"> | |
| <Dialog.Title asChild className="py-2.5 font-bold"> | |
| <span>Filters</span> | |
| </Dialog.Title> |
Summary by CodeRabbit
Release Notes
New Features
FilterItemandPriceRangeFiltercomponents for enhanced filtering options.highestPriceProductandlowestPriceProductfields to collection queries for better pricing insights.priceRangefield toPRODUCT_CARD_FRAGMENTfor comprehensive product pricing information.CartDrawercomponent for a dynamic shopping cart interface.Improvements
Checkboxcomponent's visual representation.Filterscomponent.Sortcomponent for clarity and functionality.ToolsBarcomponent by removing unnecessary complexity related to styling variants.CartCountcomponent to improve its implementation.Bug Fixes
Styles
Chores
package.json.