feat: implement FilterOptionsList component - #53
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new presentational FilterOptionsList component to render flat or recursively nested filter option trees using the existing FilterOption row component (nesting is implemented via FilterOption’s children slot). The change also tweaks FilterOption hover/group styling so nested rows don’t cause parent rows to highlight, and expands render-prop support to include children.
Changes:
- Introduces
FilterOptionsList(recursive renderer + override model) and exports it (and its types) from the package entrypoint. - Updates
FilterOptionstyling so hover/group behavior applies to the label rather than the<li>, preventing nested-hover bleed-through. - Adds Storybook examples and comprehensive Vitest coverage for recursion, indentation, overrides, and the
childrenrender-prop contract.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| src/stories/components/FilterOptionsList/FilterOptionsList.stories.tsx | Adds Storybook stories demonstrating flat/hierarchical data and the three override levels. |
| src/index.ts | Exports FilterOptionsList and related public types from the library entrypoint. |
| src/components/filter-options-list.tsx | Implements the recursive hierarchical list component and override propagation rules. |
| src/components/filter-option.tsx | Moves hover/group styling to the <label> and includes children in render-prop payload. |
| spec/components/FilterOptionsList/FilterOptionsList.test.tsx | Adds tests for recursion, indentation, selection behavior, overrides, and unique-id expectations. |
| spec/components/FilterOption/FilterOption.test.tsx | Adds a test ensuring render-prop overrides receive children for re-emitting nested content. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Code Review
This PR adds a well-designed recursive FilterOptionsList component with solid test coverage and clear documentation; the implementation is clean, the override system is consistent with the existing codebase pattern, and the HTML structure is valid.
Inline comments: 5 discussions added
Overall Assessment:
| <label | ||
| htmlFor={id} | ||
| className='cio-filter-option-label cio:text-sm cio:flex cio:flex-row cio:items-center cio:cursor-pointer cio:grow cio:p-1'> | ||
| className='cio-filter-option-label cio:group cio:text-sm cio:flex cio:flex-row cio:items-center cio:cursor-pointer cio:grow cio:p-1 cio:hover:bg-neutral-100 cio:hover:rounded'> |
There was a problem hiding this comment.
Important Issue: Moving cio:group from <li> to <label> fixes the nested-hover highlight problem, but it breaks the checkbox checked-state styling. The checkbox fill and checkmark visibility both rely on cio:group-has-[input:checked], which looks for the nearest ancestor with the group class. The <input> is a descendant of <label>, so group-has-[input:checked] on the <label> still resolves correctly — this actually works. However, the PR description should make the intent explicit in the code with a comment, and it would benefit from a targeted regression test that verifies the checkbox visually fills when isChecked=true (currently the test only checks the DOM checked attribute, not the CSS class).
Beyond the visual regression gap, there is a functional concern: cio:hover:rounded on the label rounds only the label area, but <li> still has cio:flex which means the children (nested <ul>) sits next to the label in a flex row by default. The cio:flex-col override is applied only when hasChildren is true from FilterOptionsList, but a standalone FilterOption with children passed directly will NOT receive cio:flex-col. That means a direct <FilterOption children={...}> usage would lay out the nested content in a flex row beside the label rather than below it. Add cio:flex-col as a default or document this dependency explicitly.
| nested, | ||
| ...props | ||
| }: FilterOptionsListProps & { nested: boolean }) { | ||
| const renderProps = React.useMemo( |
There was a problem hiding this comment.
Important Issue: The renderProps memo omits componentOverrides from its value, even though RenderPropsWrapper is called with props={renderProps} directly below. A render-prop consumer at the list level (e.g. componentOverrides={{ reactNode: (props) => ... }}) will receive props that includes options, onChange, className, etc., but NOT componentOverrides. This is consistent with how FilterOption handles it (also omits componentOverrides from its renderProps), but the list-level render-prop story in Storybook shows a consumer iterating props.options, so the omission is fine from a data perspective. However, if a consumer needs to forward overrides (e.g. to compose nested custom lists), they cannot. At minimum, add a JSDoc note on FilterOptionsListProps.componentOverrides that the render-prop function does not receive componentOverrides back in its props.
| checkboxPosition={checkboxPosition} | ||
| // Propagate only the row-level override — NOT the list-level `reactNode`, | ||
| // which would replace each nested list wholesale and break recursion. | ||
| componentOverrides={{ filterOption }} |
There was a problem hiding this comment.
Suggestion: This creates a new object literal { filterOption } on every render, which will cause every nested FilterOptionsListInner to receive a new componentOverrides reference each render cycle, defeating the useMemo on renderProps in the child instances. Consider memoizing the propagated componentOverrides object:
const nestedOverrides = React.useMemo(
() => ({ filterOption }),
[filterOption],
);
// then pass: componentOverrides={nestedOverrides}For small/static lists this is negligible, but for large deeply-nested trees with frequent parent re-renders it adds up.
| expect(lists[0].classList.contains('cio:pl-4')).toBeFalsy(); | ||
| }); | ||
|
|
||
| test('parent rows with children get cio:flex-col to stack the nested list below', () => { |
There was a problem hiding this comment.
Suggestion: The cio:flex-col test uses .closest('li') to find the row. For Level 0, this will find the outermost <li>, but for Level 2 it is ambiguous — getByText('Level 2').closest('li') returns the innermost <li>, which is correct here. This is fine, but the comment on why a leaf should NOT have cio:flex-col (it has no children rendered into it) would make the intent clearer and prevent future confusion.
| }); | ||
| }); | ||
|
|
||
| describe('unique-id contract', () => { |
There was a problem hiding this comment.
Suggestion: The unique-id contract test only covers the happy path (distinct ids across branches). The PR description explicitly calls out that duplicate ids break <label htmlFor> association, but there is no test for the broken case or a note that duplicate ids are the consumer's responsibility. Consider adding a comment referencing the known limitation, or a test that demonstrates the breakage when duplicate ids are provided, to serve as living documentation of the constraint.
[CDX-469] Add recursive hierarchical filter list component
Resolves CDX-469
What this adds
FilterOptionsListnow lets customers render a list of filter options, including nested lists. The component takes plain data and presents it - no business logic like restructuring, fetching, or knowing about Constructor.io facets or PLP. Customers map their own data into the shape and it renders it.The library already had
FilterOptionfor a single row, but nothing to render a list of them and nothing that handled nesting. This adds that.How it works
Nesting reuses
FilterOption'schildrenslot. Each option renders aFilterOption. If it hashierarchies, the nested list is rendered into that row'schildren.FilterOptionalready renders{children}after its label, so a nested<ul>inside the<li>is valid HTML and needs no new nesting part. Depth is unlimited and the indentation adds up on its own through nesting - each level just adds one step of padding (cio:pl-4), so there's no depth counter to track. The root list isn't indented; every nested one is.The data type is
Picked fromFilterOptionProps.FilterOptionDatatakes only the display fields (id,optionValue,displayValue, etc.) fromFilterOptionPropsand adds the nestedhierarchies. IfFilterOptionrenames a prop, this breaks at compile time instead of silently. One thing to know, and it's noted in the type:ids must be unique across the whole tree, becauseFilterOptionusesidto pair its<input id>with its<label htmlFor>. Duplicate ids across branches would let one click toggle the wrong row. There's a test for this.Public wrapper + private recursive renderer.
FilterOptionsListis a thin wrapper that calls an internalFilterOptionsListInnerwith anestedflag. The recursion needs to know if it's the root or a nested list to decide indentation, but I didn't want that flag in the public props (where it would show up in autodocs and be settable by customers). So it lives only on the private inner component.Overrides - three levels
componentOverridesworks at three levels:reactNodereplaces the whole<ul>. The render-prop form gets the list's props (options,onChange, …), so you can lay it out your own way off the same data.filterOptionas an object applies to every row at every level.filterOptionas a function runs per option. Return an override for the rows you want andundefinedfor the rest, e.g.(option) => option.id === 'x' ? {...} : undefined.filter-option.tsx- two small changes<li>to the<label>. With hover on the<li>, hovering a nested child row also highlighted its parents, since the child<li>sits inside the parent<li>. Moving hover (andcio:group, which the checkbox's checked styling relies on) to the<label>means only the row under the cursor lights up. The standaloneFilterOptionlooks the same and its tests still pass.childrento itsrenderProps. This is what lets a render-prop override re-emit a row's nested list. Added a test for it.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?