-
Notifications
You must be signed in to change notification settings - Fork 2
[CDX-326] Allow blacklisting facets from filters #207
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.
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
base: main
Are you sure you want to change the base?
[CDX-326] Allow blacklisting facets from filters #207
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds functionality to allow facets and facet options to be hidden from the filter UI through three methods: custom functions, metadata flags, or custom field getters. This enables conditional rendering of filters on a per-page basis (e.g., showing a facet on search results but hiding it on category pages).
Changes:
- Added
isHiddenFilterFnandisHiddenFilterOptionFnprops to filter components for custom hiding logic - Introduced
getIsHiddenFilterFieldandgetIsHiddenFilterOptionFieldfield getters to support metadata-based hiding viacio_plp_hiddenflag - Enhanced
useOptionsListhook with recursive filtering capability for nested options in hierarchical structures
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/itemFieldGetters.ts | Added field getter functions for detecting hidden filters and filter options |
| src/types.ts | Updated type definitions to support new field getters and changed data type from object to Record<string, any> |
| src/stories/components/Filters/Filters.stories.tsx | Added story examples demonstrating the three methods for hiding filters |
| src/stories/components/Filters/Code Examples.mdx | Added comprehensive documentation with code examples for the hiding functionality |
| src/hooks/useOptionsList.ts | Added recursive filtering support for nested options via nestedOptionsKey parameter |
| src/hooks/useGroups.ts | Enabled recursive filtering for group children by passing nestedOptionsKey: 'children' |
| src/hooks/useFilter.ts | Implemented facet filtering logic using both custom function and metadata-based approaches |
| src/components/Filters/UseFilterOptionsList.tsx | Added support for hiding individual filter options using custom functions and metadata |
| src/components/Filters/Filters.tsx | Passed through isHiddenFilterOptionFn prop to child components |
| src/components/Filters/FilterGroup.tsx | Passed through isHiddenFilterOptionFn prop to FilterOptionsList |
| spec/hooks/useGroups/useGroups.test.js | Added tests for recursive filtering of nested group children |
| spec/hooks/useFilter/useFilter.test.js | Added comprehensive tests for facet hiding functionality |
| spec/components/Filters/Filters.test.jsx | Added integration tests for filter and filter option hiding |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| displayName: string; | ||
| value: string; | ||
| data: object; | ||
| data: Record<string, any>; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using any bypasses TypeScript's type safety. Consider using unknown instead, which provides type safety while still allowing flexibility: Record<string, unknown>.
| export interface PlpHierarchicalFacetOption extends PlpFacetOption { | ||
| options: Array<PlpHierarchicalFacetOption>; | ||
| data: object & { parentValue: string | null }; | ||
| data: Record<string, any> & { parentValue: string | null }; |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using any bypasses TypeScript's type safety. Consider using unknown instead: Record<string, unknown> & { parentValue: string | null }.
| data: Record<string, any> & { parentValue: string | null }; | |
| data: Record<string, unknown> & { parentValue: string | null }; |
| const nestedOptions = (option as Record<string, any>)[nestedKey]; | ||
| if (Array.isArray(nestedOptions) && nestedOptions.length > 0) { | ||
| return { | ||
| ...option, | ||
| [nestedKey]: filterOptionsRecursively(nestedOptions, isHiddenFn, nestedKey), |
Copilot
AI
Jan 28, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using any bypasses TypeScript's type safety. Consider using unknown instead and adding proper type guards: (option as Record<string, unknown>)[nestedKey].
| const nestedOptions = (option as Record<string, any>)[nestedKey]; | |
| if (Array.isArray(nestedOptions) && nestedOptions.length > 0) { | |
| return { | |
| ...option, | |
| [nestedKey]: filterOptionsRecursively(nestedOptions, isHiddenFn, nestedKey), | |
| const nestedOptions = (option as Record<string, unknown>)[nestedKey]; | |
| if (Array.isArray(nestedOptions) && nestedOptions.length > 0) { | |
| return { | |
| ...option, | |
| [nestedKey]: filterOptionsRecursively(nestedOptions as Array<T>, isHiddenFn, nestedKey), |
Code Review Results✅ StrengthsWell-structured feature with comprehensive test coverage, clear documentation, and thoughtful API design that supports multiple hiding strategies (custom functions, metadata field, and custom field getters). 🚨 Critical Issues[File: Recommendation: Use proper type guards or optional chaining: function filterOptionsRecursively<T>(
options: Array<T>,
isHiddenFn: (option: T) => boolean,
nestedKey: string,
): Array<T> {
return options
.filter((option) => !isHiddenFn(option))
.map((option) => {
const nestedOptions = (option as any)?.[nestedKey];
if (Array.isArray(nestedOptions) && nestedOptions.length > 0) {
return {
...option,
[nestedKey]: filterOptionsRecursively(nestedOptions, isHiddenFn, nestedKey),
};
}
return option;
});
}[File: Recommendation: Filter out hidden options before building the selected map: useEffect(() => {
const newSelectedOptionsMap = {};
const visibleOptions = facet.options.filter((option) => !isHiddenOptionFn(option));
visibleOptions.forEach((option) => {
newSelectedOptionsMap[option.value] = option.status === 'selected';
});
setSelectedOptionMap(newSelectedOptionsMap);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [facet, isHiddenOptionFn]);
|
Pull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?
Clone of #192