Add option to display color filters as swatches #230
Conversation
WalkthroughThis pull request introduces several modifications across various components in the application. The Changes
Possibly related PRs
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: 2
🧹 Outside diff range and nitpick comments (3)
app/sections/collection-filters/index.tsx (1)
187-187: Consider enhancing the help text for displayAsButtonFor.While the help text is informative, it could be more specific about the expected format and provide examples.
- helpText: "Comma-separated list of filters to display as buttons", + helpText: "Comma-separated list of filters to display as buttons (e.g., 'Size, Color, Material')",app/sections/collection-filters/filters.tsx (2)
57-58: Consider making color filter detection more flexible.The current implementation uses exact matches for color filter labels. Consider using case-insensitive comparison or allowing custom color filter labels through configuration.
-const COLORS_FILTERS = ["Color", "Colors", "Colour", "Colours"]; +const COLORS_FILTERS = ["color", "colors", "colour", "colours"]; - enableColorSwatch && COLORS_FILTERS.includes(filter.label); + enableColorSwatch && COLORS_FILTERS.includes(filter.label.toLowerCase());Also applies to: 88-90
153-162: Consider debouncing filter changes.The filter change handler might trigger frequent navigation updates. Consider implementing debouncing for better performance.
+import { useDebounce } from '~/hooks/use-debounce'; + function ListItemFilter({ ... }) { + const debouncedNavigate = useDebounce(navigate, 300); + function handleCheckedChange(checked: boolean) { setChecked(checked); if (checked) { let link = getFilterLink(option.input as string, params, location); - navigate(link, { preventScrollReset: true }); + debouncedNavigate(link, { preventScrollReset: true }); } else if (filter) { let link = getAppliedFilterLink(filter, params, location); - navigate(link, { preventScrollReset: true }); + debouncedNavigate(link, { preventScrollReset: true }); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/components/checkbox.tsx(1 hunks)app/components/tooltip.tsx(1 hunks)app/modules/product-form/options.tsx(1 hunks)app/sections/collection-filters/filters.tsx(2 hunks)app/sections/collection-filters/index.tsx(3 hunks)tailwind.config.js(2 hunks)
🔇 Additional comments (11)
app/components/checkbox.tsx (3)
8-8: LGTM: Label type change enables color swatch rendering
The change from string to React.ReactNode aligns with the PR objective of supporting color swatches in filters.
15-15: LGTM: UI refinements
Good improvements:
- Using
gapinstead ofspace-xis more modern - Simplified checkbox styles while maintaining functionality
- Updated to Phosphor icons maintains consistency
Also applies to: 18-18, 21-22
25-25: LGTM: Flexible label rendering
The conditional rendering handles both string labels and component labels (like color swatches) appropriately.
app/components/tooltip.tsx (2)
27-27: LGTM: Flexible animation customization
Good practices:
- Added style prop for custom animation timing
- Default duration fallback in Tailwind config
- Proper spreading of additional styles
Also applies to: 40-45
33-33: LGTM: Clean tooltip styling
The tooltip styling provides good contrast with background/text colors and appropriate spacing.
tailwind.config.js (2)
34-40: LGTM: Well-structured animation keyframe
Good implementation:
- Uses CSS variable for customizable distance
- Handles both opacity and transform
- Matches existing animation patterns
85-86: LGTM: Flexible animation configuration
Well implemented:
- Customizable duration via CSS variable
- Smooth easing with cubic-bezier
- Consistent with other animation configurations
app/sections/collection-filters/index.tsx (2)
18-19: LGTM! Interface updates are well-defined.
The new properties enableColorSwatch and displayAsButtonFor are clearly typed and appropriately placed in the interface.
37-38: LGTM! Props destructuring is correctly implemented.
The new props are properly destructured from the parent data object.
app/modules/product-form/options.tsx (1)
9-43: Verify the usage of exported variants across components.
The variants configuration is now exported, which is good for reusability. However, we should ensure this doesn't lead to unexpected styling conflicts in other components.
✅ Verification successful
No styling conflicts found with the exported variants
The exported variants from product-form/options.tsx is only imported once in collection-filters/filters.tsx where it's renamed to productOptionsVariants to avoid naming conflicts. The other occurrences of variants in the codebase are either:
- Different variant configurations defined locally in their respective components
- GraphQL query references related to product variants
The current implementation follows good practices by:
- Using a descriptive alias when importing to avoid naming collisions
- Keeping variant configurations scoped to their respective components
- Having clear separation between styling variants and product variants
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for imports of variants from this file
rg "import.*variants.*from.*product-form/options" -A 2
# Search for direct usage of the variants
rg "variants\({.*}\)" -A 2
Length of output: 478
Script:
#!/bin/bash
# Search for imports using a more flexible pattern
rg "from.*product-form/options"
# Search for variant usage with a corrected pattern
rg "variants\(" -A 2
# Search for className usage that might reference variants
rg "className.*variants" -A 2
Length of output: 32343
app/sections/collection-filters/filters.tsx (1)
25-29: LGTM! Dependencies and constants are well-organized.
The new imports and COLORS_FILTERS constant are appropriately defined. The constant helps standardize color filter detection.
| } | ||
|
|
||
| const Checkbox = React.forwardRef< | ||
| export let Checkbox = React.forwardRef< |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider keeping the export as const
Using let for component exports is unusual and suggests potential mutation, which could lead to unexpected behavior. React components should generally be immutable.
-export let Checkbox = React.forwardRef<
+export const Checkbox = React.forwardRef<📝 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.
| export let Checkbox = React.forwardRef< | |
| export const Checkbox = React.forwardRef< |
| if (asColorSwatch) { | ||
| let swatchColor = swatches.colors.find(({ name }) => name === option.label); | ||
| let optionConf = options.find(({ name }) => { | ||
| return name.toLowerCase() === option.label.toLowerCase(); | ||
| }); | ||
|
|
||
| let { shape = "square", size = "md" } = optionConf || {}; | ||
| return ( | ||
| <Tooltip> | ||
| <TooltipTrigger> | ||
| <button | ||
| type="button" | ||
| className={cn( | ||
| productOptionsVariants({ | ||
| colorSize: size, | ||
| shape, | ||
| }), | ||
| checked ? "p-1 border-line" : "border-line-subtle", | ||
| )} | ||
| onClick={() => handleCheckedChange(!checked)} | ||
| > | ||
| <span | ||
| className={cn( | ||
| "w-full h-full inline-block border-none hover:border-none", | ||
| productOptionsVariants({ shape }), | ||
| )} | ||
| style={{ | ||
| backgroundColor: | ||
| swatchColor?.value || option.label.toLowerCase(), | ||
| }} | ||
| /> | ||
| </button> | ||
| </TooltipTrigger> | ||
| <TooltipContent> | ||
| {showFiltersCount ? ( | ||
| <span> | ||
| {option.label}{" "} | ||
| <span className="text-gray-100">({option.count})</span> | ||
| </span> | ||
| ) : ( | ||
| option.label | ||
| )} | ||
| </TooltipContent> | ||
| </Tooltip> | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance color swatch accessibility.
The color swatch implementation should include ARIA attributes for better accessibility. Also, consider adding a fallback color for invalid color values.
<button
type="button"
+ aria-label={`${option.label} (${option.count} items)`}
className={cn(
productOptionsVariants({
colorSize: size,
shape,
}),
checked ? "p-1 border-line" : "border-line-subtle",
)}
onClick={() => handleCheckedChange(!checked)}
>
<span
className={cn(
"w-full h-full inline-block border-none hover:border-none",
productOptionsVariants({ shape }),
)}
style={{
backgroundColor:
- swatchColor?.value || option.label.toLowerCase(),
+ swatchColor?.value || option.label.toLowerCase() || '#cccccc',
}}
/>
</button>📝 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.
| if (asColorSwatch) { | |
| let swatchColor = swatches.colors.find(({ name }) => name === option.label); | |
| let optionConf = options.find(({ name }) => { | |
| return name.toLowerCase() === option.label.toLowerCase(); | |
| }); | |
| let { shape = "square", size = "md" } = optionConf || {}; | |
| return ( | |
| <Tooltip> | |
| <TooltipTrigger> | |
| <button | |
| type="button" | |
| className={cn( | |
| productOptionsVariants({ | |
| colorSize: size, | |
| shape, | |
| }), | |
| checked ? "p-1 border-line" : "border-line-subtle", | |
| )} | |
| onClick={() => handleCheckedChange(!checked)} | |
| > | |
| <span | |
| className={cn( | |
| "w-full h-full inline-block border-none hover:border-none", | |
| productOptionsVariants({ shape }), | |
| )} | |
| style={{ | |
| backgroundColor: | |
| swatchColor?.value || option.label.toLowerCase(), | |
| }} | |
| /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| {showFiltersCount ? ( | |
| <span> | |
| {option.label}{" "} | |
| <span className="text-gray-100">({option.count})</span> | |
| </span> | |
| ) : ( | |
| option.label | |
| )} | |
| </TooltipContent> | |
| </Tooltip> | |
| ); | |
| } | |
| if (asColorSwatch) { | |
| let swatchColor = swatches.colors.find(({ name }) => name === option.label); | |
| let optionConf = options.find(({ name }) => { | |
| return name.toLowerCase() === option.label.toLowerCase(); | |
| }); | |
| let { shape = "square", size = "md" } = optionConf || {}; | |
| return ( | |
| <Tooltip> | |
| <TooltipTrigger> | |
| <button | |
| type="button" | |
| aria-label={`${option.label} (${option.count} items)`} | |
| className={cn( | |
| productOptionsVariants({ | |
| colorSize: size, | |
| shape, | |
| }), | |
| checked ? "p-1 border-line" : "border-line-subtle", | |
| )} | |
| onClick={() => handleCheckedChange(!checked)} | |
| > | |
| <span | |
| className={cn( | |
| "w-full h-full inline-block border-none hover:border-none", | |
| productOptionsVariants({ shape }), | |
| )} | |
| style={{ | |
| backgroundColor: | |
| swatchColor?.value || option.label.toLowerCase() || '#cccccc', | |
| }} | |
| /> | |
| </button> | |
| </TooltipTrigger> | |
| <TooltipContent> | |
| {showFiltersCount ? ( | |
| <span> | |
| {option.label}{" "} | |
| <span className="text-gray-100">({option.count})</span> | |
| </span> | |
| ) : ( | |
| option.label | |
| )} | |
| </TooltipContent> | |
| </Tooltip> | |
| ); | |
| } |
Summary by CodeRabbit
New Features
Checkboxcomponent to support flexible label content.TooltipContentcomponent.Filterscomponent.CollectionFilters.Bug Fixes
Chores