[CDX-336] Add visual filter support and replace filter option component#219
[CDX-336] Add visual filter support and replace filter option component#219
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds “visual filter” (swatch/image) rendering support for PLP facets and replaces the in-repo filter option row implementation with components from @constructor-io/constructorio-ui-components.
Changes:
- Introduces visual-filter resolution helpers and per-facet visual rendering overrides (
filterConfigs, callbacks). - Updates Filters/Groups rendering to use
constructorio-ui-components(FilterOption,FilterOptionVisual) and wires visual props through hooks/components. - Adds/updates Storybook docs + fixtures and adds unit/component test coverage for the new visual behavior.
Reviewed changes
Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/visualFilterHelpers.ts | Adds helper logic for determining visual facets + resolving option visuals. |
| src/utils/index.ts | Re-exports the new visual filter helpers. |
| src/types.ts | Adds FilterConfig type for per-facet overrides. |
| src/styles.css | Imports base styles from @constructor-io/constructorio-ui-components. |
| src/hooks/useFilter.ts | Extends filter hook props/return with visual callbacks + per-facet config map. |
| src/components/Filters/FilterGroup.tsx | Computes isVisual per facet and passes visual props into option list rendering. |
| src/components/Filters/UseFilterOptionsList.tsx | Plumbs isVisual + visual resolver callbacks through the options list hook. |
| src/components/Filters/FilterOptionsList.tsx | Renders visual vs standard filter options using UI components + helper resolution. |
| src/components/Filters/FilterOptionsList.css | Removes CSS tied to the deleted in-repo option row component. |
| src/components/Filters/FilterOptionListRow.tsx | Deletes the custom option row component replaced by UI components. |
| src/components/Filters/Filters.tsx | Wires new visual props from useFilter into FilterGroup / render-props. |
| src/components/Groups/Groups.tsx | Replaces group option row rendering with UI component FilterOption. |
| src/components/CioPlpGrid/CioPlpGrid.tsx | Passes filter config props through to Filters in both desktop and modal layouts. |
| src/index.ts | Exports UseFilterReturn / UseFilterProps types from the package entrypoint. |
| src/stories/components/Filters/Filters.stories.tsx | Adds visual-filter story variants and mock facet option metadata. |
| src/stories/components/CioPlp/CioPlp.stories.tsx | Demonstrates passing visual filter callbacks via filterConfigs. |
| src/stories/components/CioPlp/Props.mdx | Documents the new visual filter-related config fields. |
| src/stories/hooks/UseFilter/UseFilterReturn.md | Documents new fields added to UseFilterReturn. |
| spec/utils/visualFilterHelpers.test.ts | Adds unit tests for the new visual filter helpers. |
| spec/components/Filters/Filters.test.jsx | Adds component tests verifying visual option rendering + overrides. |
| spec/local_examples/sampleFacets.json | Updates fixture values (“No Colour” → “No Color”). |
| spec/local_examples/apiSearchResponse.json | Updates fixture values (“No Colour” → “No Color”). |
| package.json | Adds @constructor-io/constructorio-ui-components dependency. |
| package-lock.json | Locks the new dependency and its transitive dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Adds “visual filter” rendering support (color/image swatches) to the Filters UI by introducing helper utilities and wiring new callbacks/config into the filter pipeline, while also replacing the bespoke filter option row with @constructor-io/constructorio-ui-components.
Changes:
- Introduces
shouldRenderVisualFacet/resolveVisualOptionhelpers and threads new visual-filter config callbacks throughuseFilter→Filters→FilterGroup→FilterOptionsList. - Replaces the internal filter option row implementation with
FilterOption/FilterOptionVisualcomponents and imports the shared UI component stylesheet. - Updates Storybook docs/stories and adds tests covering visual filter behavior; bumps Node engine requirement and adds the UI components dependency.
Reviewed changes
Copilot reviewed 27 out of 28 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/visualFilterHelpers.ts | Adds visual facet decision + option visual resolution helpers. |
| src/utils/index.ts | Re-exports the new visual filter helpers. |
| src/types.ts | Adds FacetConfig for per-facet overrides. |
| src/styles.css | Imports shared UI component styles globally. |
| src/hooks/useFilter.ts | Extends hook props/return to include visual filter callbacks/config. |
| src/hooks/useCioPlp.ts | Updates filterConfigs doc comment (removes “no configs yet”). |
| src/components/Filters/Filters.tsx | Passes visual filter config through render props + FilterGroup. |
| src/components/Filters/FilterGroup.tsx | Determines “visual facet” per facet and passes flags/callbacks down. |
| src/components/Filters/UseFilterOptionsList.tsx | Carries visual flags/callbacks through the options list hook. |
| src/components/Filters/FilterOptionsList.tsx | Renders FilterOptionVisual when visual data is available; otherwise FilterOption. |
| src/components/Filters/FilterOptionsList.css | Removes legacy option-row styling now owned by shared UI components. |
| src/components/Groups/Groups.tsx | Uses shared FilterOption component instead of local row component. |
| src/components/CioPlpGrid/CioPlpGrid.tsx | Threads filterConfigs into Filters in the grid. |
| src/index.ts | Exports UseFilterReturn / UseFilterProps types. |
| src/stories/**/* | Adds/updates stories + docs demonstrating visual filters and new config props. |
| spec/utils/visualFilterHelpers.test.ts | Adds unit tests for the new helper utilities. |
| spec/components/Filters/Filters.test.jsx | Adds integration-ish tests for visual filter rendering. |
| spec/local_examples/*.json | Normalizes “No Colour” → “No Color” in fixtures. |
| package.json / package-lock.json | Adds @constructor-io/constructorio-ui-components, broadens license allowlist, bumps Node engine to >=20. |
| .nvmrc | Removes pinned Node version file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
| getVisualImageUrl?: (option: PlpFacetOption) => string | undefined; | ||
| getVisualColorHex?: (option: PlpFacetOption) => string | undefined; | ||
| isVisualFilterFn?: (facet: PlpFacet) => boolean; | ||
| perFacetConfigs?: Record<string, FacetConfig>; |
There was a problem hiding this comment.
I don't love perFacetConfigs and I initially made this filterConfigs but we have a top level prop with the same name so I switched back
This comment was marked as outdated.
This comment was marked as outdated.
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
HHHindawy
left a comment
HHHindawy
left a comment
There was a problem hiding this comment.
Thanks for working on this, it looks great!
I just left you a couple of comments that are open for discussion if needed 🙏
|
Looks like the License Check failed |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* [CDX-285] Add support for default collapsing of individual filter groups * [CDX-285] Add sample facets with default collapsing metadata * [CDX-285] refactor & simplify filter group collapse logic in tests and components * [CDX-285] test: wrap FilterGroup component in CioPlp for API key context * [CDX-285] feat: implement per-facet configuration for collapsing filter groups * [CDX-285] fix: add missing comma in Filters.tsx for proper syntax * [CDX-285] feat: refactor color facet logic and update visual filter configurations * [CDX-285] feat: enhance filter collapsing logic and add color constants * [CDX-285] feat: update filter collapsing configuration to use defaultCollapsed instead of renderCollapsed * [CDX-285] feat: enhance defaultCollapsed documentation and implement custom collapse behavior for facets --------- Co-authored-by: Enes Kutay SEZEN <eneskutaysezen@gmail.com>
There was a problem hiding this comment.
Pull request overview
This PR introduces “visual filter” rendering (color swatches / image thumbnails) and adds configurable collapsed-by-default behavior for facet groups, while also replacing the in-repo filter option row component with shared UI components from @constructor-io/constructorio-ui-components.
Changes:
- Add visual facet/option resolution helpers and wire them through
useFilter→Filters→FilterGroup→FilterOptionsList. - Add facet collapse behavior driven by per-facet config, facet metadata, or a global default.
- Replace filter/group option rows with shared UI components and import shared component styles; bump Node engine and add the new dependency.
Reviewed changes
Copilot reviewed 35 out of 36 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/visualFilterHelpers.ts | Adds helpers to determine visual facet rendering and resolve visual option payloads. |
| src/utils/itemFieldGetters.ts | Adds getIsCollapsedFacetField getter (metadata-based collapse). |
| src/utils/index.ts | Re-exports visualFilterHelpers. |
| src/types.ts | Adds FacetConfig and extends ItemFieldGetters with getIsCollapsedFacetField. |
| src/styles.css | Imports shared UI components stylesheet before existing imports. |
| src/stories/utils/colorConstants.ts | Adds story-only color name → hex map constants. |
| src/stories/hooks/UseFilter/UseFilterReturn.md | Documents new useFilter return fields (visual + collapse). |
| src/stories/components/Filters/Filters.stories.tsx | Adds stories demonstrating visual filters and collapsed facets. |
| src/stories/components/Filters/Code Examples.mdx | Documents filterConfigs (collapse + visual filters) with examples. |
| src/stories/components/CioPlpGrid/Code Examples.mdx | Documents passing filterConfigs through CioPlpGrid. |
| src/stories/components/CioPlp/Props.mdx | Updates prop docs for new filter configs and collapsed getter. |
| src/stories/components/CioPlp/Code Examples.mdx | Adds filterConfigs usage example and minor formatting tweaks. |
| src/stories/components/CioPlp/CioPlp.stories.tsx | Demonstrates default visual filter callbacks via filterConfigs. |
| src/index.ts | Exports UseFilterReturn / UseFilterProps types. |
| src/hooks/useFilter.ts | Adds visual filter callbacks, perFacetConfigs, and getIsCollapsed resolver. |
| src/hooks/useCioPlp.ts | Updates filterConfigs doc comment and continues to pass configs into useFilter. |
| src/components/Groups/Groups.tsx | Replaces option row with shared FilterOption component (checkbox hidden). |
| src/components/Filters/UseFilterOptionsList.tsx | Threads visual/checkbox props through the filter options list hook. |
| src/components/Filters/Filters.tsx | Passes visual configs + computed collapse default down to FilterGroup. |
| src/components/Filters/FilterOptionsList.tsx | Renders shared FilterOption/FilterOptionVisual based on resolved visuals. |
| src/components/Filters/FilterOptionsList.css | Removes old checkbox styles; keeps a small backward-compat label fix. |
| src/components/Filters/FilterOptionListRow.tsx | Removes the legacy in-repo option row component. |
| src/components/Filters/FilterGroup.tsx | Adds initial collapsed state + visual rendering + checkbox position per facet. |
| src/components/CioPlpGrid/CioPlpGrid.tsx | Passes filterConfigs through to Filters; updates doc comment. |
| spec/utils/visualFilterHelpers.test.ts | Unit tests for visual facet/option helper priority logic. |
| spec/local_examples/sampleFacetsWithCollapsedMetadata.json | Adds sample facets fixture with cio_render_collapsed. |
| spec/local_examples/sampleFacets.json | Normalizes “No Colour” → “No Color” in sample facet fixture. |
| spec/local_examples/apiSearchResponse.json | Normalizes “No Colour” → “No Color” in sample API fixture. |
| spec/hooks/useFilter/useFilter.test.js | Adds tests for getIsCollapsed priority and custom getter behavior. |
| spec/components/Filters/Filters.test.jsx | Adds visual filter and collapse behavior tests; minor assertion fixes. |
| spec/components/Filters/Filters.server.test.jsx | Adds SSR tests for default collapsed/expanded behavior. |
| spec/components/Filters/FilterGroup.test.jsx | Adds tests for FilterGroup honoring defaultCollapsed. |
| spec/components/CioPlp/CioPlp.test.jsx | Adds client tests for filterConfigs.defaultCollapsed. |
| package.json | Adds UI components dependency, bumps Node engine, updates license allowlist. |
| package-lock.json | Locks new dependency tree and reflects Node engine bump. |
| .nvmrc | Removes the repo’s Node version file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Alexey-Pavlov
left a comment
Alexey-Pavlov
left a comment
There was a problem hiding this comment.
Thank you for working on this! It looks great! Left just a few minor comments here and there
| .cio-filter-option-count { | ||
| color: #999; | ||
| margin-left: 8px; | ||
| /* Backwards compatibility temporary fix - will be removed in the next major version */ |
There was a problem hiding this comment.
| /* Backwards compatibility temporary fix - will be removed in the next major version */ | |
| /* TODO: Backwards compatibility temporary fix - will be removed in the next major version */ |
Sometimes, as a lowly WebStorm user who doesn't use Vim, TODOs really save me. When I try to commit something with a TODO, WebStorm stops the commit and gives me a chance to think twice about whether I actually want to do something about it, haha
There was a problem hiding this comment.
@Alexey-Pavlov I'm down to make it a "TODO" but I would like to keep the original message. I don't think we will add the line-height to ui-components we would just remove this line and accept a slightly shift in the UI in the next major version. Wdyt?
Co-authored-by: Aleksei Pavlov <pavlov.aleksey.m@gmail.com>
Co-authored-by: Aleksei Pavlov <pavlov.aleksey.m@gmail.com>
Co-authored-by: Aleksei Pavlov <pavlov.aleksey.m@gmail.com>
Co-authored-by: Aleksei Pavlov <pavlov.aleksey.m@gmail.com>
Co-authored-by: Aleksei Pavlov <pavlov.aleksey.m@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This PR adds visual filter support (image/color swatches), per-facet config overrides, and collapse behavior to the filter components, replacing the internal FilterOptionListRow with components from the new @constructor-io/constructorio-ui-components dependency — a solid, well-tested feature addition with thorough test coverage.
Inline comments: 7 discussions added
Overall Assessment:
| "webpack": "^5.94.0" | ||
| }, | ||
| "dependencies": { | ||
| "@constructor-io/constructorio-ui-components": "^1.3.2", |
There was a problem hiding this comment.
Important Issue: @constructor-io/constructorio-ui-components is added as a production dependency rather than a peerDependency. This library ships both tailwindcss (^4.1.x) and embla-carousel-react as runtime dependencies, which will be bundled into every consumer's build. Since this package is a UI library itself, heavyweight runtime dependencies like Tailwind's toolchain (@tailwindcss/vite, @tailwindcss/oxide, and their ~20 platform-specific binaries) will bloat the distributed bundle and may cause version conflicts with the host application. Consider making @constructor-io/constructorio-ui-components a peerDependency (or at minimum carefully auditing what actually ends up in the compiled lib/ output) to avoid shipping Tailwind's build tooling as a transitive runtime dependency.
| checkboxPosition, | ||
| }; | ||
|
|
||
| if (isVisual) { |
There was a problem hiding this comment.
Important Issue: When a facet is marked isVisual but an option has no resolvable visual data (i.e., resolveVisualOption returns null), the code silently falls back to <FilterOption> (the standard checkbox row). This silent fallback is intentional per the design, but it is not obvious to consumers who set cio_render_visual: true on a facet. Consider logging a console.warn when visual rendering is requested but no visual data can be resolved for an option, to help developers debug misconfigured option data.
| } | ||
|
|
||
| const { getIsHiddenFilterField } = contextValue.itemFieldGetters; | ||
| const { getIsHiddenFilterField, getIsCollapsedFacetField } = contextValue.itemFieldGetters; |
There was a problem hiding this comment.
Suggestion: setFilter directly mutates the newFilters object obtained from getRequestConfigs().filters:
const newFilters = getRequestConfigs().filters || {};
newFilters[filterName] = filterValue; // mutates the refIf getRequestConfigs().filters returns a reference to internal state, this is a direct mutation of that state object before setRequestConfigs is called. Per the coding guidelines, utility functions should return new objects instead of modifying parameters. This pre-existing pattern is now more prominent with the addition of new APIs in this PR. Prefer const newFilters = { ...(getRequestConfigs().filters || {}) }; to avoid subtle mutation bugs.
| const [isCollapsed, setIsCollapsed] = useState(false); | ||
| const [isCollapsed, setIsCollapsed] = useState(defaultCollapsed); | ||
| const isVisual = shouldRenderVisualFacet(facet, perFacetConfigs, isVisualFilterFn); | ||
| const checkboxPosition = perFacetConfigs?.[facet.name]?.checkboxPosition; |
There was a problem hiding this comment.
Suggestion: checkboxPosition is extracted from perFacetConfigs in FilterGroup and then passed to FilterOptionsList, but isVisual (derived from shouldRenderVisualFacet) is also passed. When isVisual is true and checkboxPosition is e.g. 'left', the checkboxPosition is passed to FilterOptionVisual via commonProps. It is worth confirming (and documenting) whether FilterOptionVisual from @constructor-io/constructorio-ui-components honours the checkboxPosition prop at all — if not, the prop is silently ignored for visual options, which could confuse consumers who set checkboxPosition on a visual facet.
| @@ -1 +0,0 @@ | |||
| v18.13.0 | |||
There was a problem hiding this comment.
Suggestion: .nvmrc is deleted and replaced with the "volta" config in package.json (node: 22.18.0). The engines field in package.json also jumps from >=14 to >=20. These are significant, potentially breaking changes for CI environments or contributors relying on .nvmrc. This is fine if intentional (Tailwind v4 requires Node >=20), but the PR description does not mention it. Ensure CI pipelines and the README are updated to reflect the new minimum Node version requirement.
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?