Skip to content

feat: implement FilterOptionsList component - #53

Open
Sher-Bakhodirov wants to merge 2 commits into
mainfrom
cdx-469-components-ui-add-recursive-hierarchical-filter-list
Open

feat: implement FilterOptionsList component#53
Sher-Bakhodirov wants to merge 2 commits into
mainfrom
cdx-469-components-ui-add-recursive-hierarchical-filter-list

Conversation

@Sher-Bakhodirov

@Sher-Bakhodirov Sher-Bakhodirov commented Aug 10, 2026

Copy link
Copy Markdown

[CDX-469] Add recursive hierarchical filter list component

Resolves CDX-469

What this adds

FilterOptionsList now 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 FilterOption for 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's children slot. Each option renders a FilterOption. If it has hierarchies, the nested list is rendered into that row's children. FilterOption already 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 from FilterOptionProps. FilterOptionData takes only the display fields (id, optionValue, displayValue, etc.) from FilterOptionProps and adds the nested hierarchies. If FilterOption renames 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, because FilterOption uses id to 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. FilterOptionsList is a thin wrapper that calls an internal FilterOptionsListInner with a nested flag. 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

componentOverrides works at three levels:

  1. The whole list - reactNode replaces 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.
  2. Every row - filterOption as an object applies to every row at every level.
  3. A single row - filterOption as a function runs per option. Return an override for the rows you want and undefined for the rest, e.g. (option) => option.id === 'x' ? {...} : undefined.

filter-option.tsx - two small changes

  1. Moved hover styling from the <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 (and cio:group, which the checkbox's checked styling relies on) to the <label> means only the row under the cursor lights up. The standalone FilterOption looks the same and its tests still pass.
  2. Added children to its renderProps. 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:

  • I have added or updated TypeScript types for my changes, ensuring they are compatible with the existing codebase.
  • I have added JSDoc comments to my TypeScript definitions for improved documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added any necessary documentation (if appropriate).
  • I have made sure my PR is up-to-date with the main branch.

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no API changes)
  • Documentation content changes
  • TypeScript type definitions update
  • Other... Please describe:

@Sher-Bakhodirov
Sher-Bakhodirov requested review from a team, Mudaafi and esezen and a lite review from Copilot August 10, 2026 13:26
@Sher-Bakhodirov
Sher-Bakhodirov requested a review from a team as a code owner August 10, 2026 13:26
constructor-claude-bedrock[bot]

This comment was marked as outdated.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 FilterOption styling 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 children render-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.

@constructor-claude-bedrock constructor-claude-bedrock Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ⚠️ Needs Work

<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'>

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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', () => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants