-
Notifications
You must be signed in to change notification settings - Fork 6
fix(FilterPicker): full items prop support #737
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
Conversation
🦋 Changeset detectedLatest commit: fd6cd1e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📦 NPM canary releaseDeployed canary version 0.0.0-canary-220b76c. |
🧪 Storybook is successfully deployed!
|
🏋️ Size limit report
Click here if you want to find out what is changed in this build |
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 enhances the FilterPicker component to provide full support for the items
prop, enabling data-driven rendering without requiring children elements. This addresses limitations where the FilterPicker had incomplete support for items-based configuration.
- Added
ItemWithKey
interface to properly type items with keys, IDs, and children - Implemented comprehensive label extraction and sorting logic for items-based data
- Enhanced type safety by replacing
any
types with proper type annotations
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
FilterPicker.tsx | Core implementation adding items prop support with sorting, label extraction, and type safety improvements |
FilterPicker.stories.tsx | Updated stories to demonstrate items prop usage and fixed test data setup |
FilterListBox.stories.tsx | Minor test data adjustment for consistency |
Button.tsx | Simplified icon rendering logic |
.changeset/metal-pumpkins-appear.md | Added changeset for the patch release |
|
||
/** Ref to access internal ListBox state (from FilterListBox) */ | ||
listStateRef?: MutableRefObject<any | null>; | ||
listStateRef?: MutableRefObject<unknown>; |
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.
The type change from any | null
to unknown
is too restrictive for a ref that should hold state objects. Consider using MutableRefObject<ListState<T> | null>
or a more specific state type instead of unknown
.
listStateRef?: MutableRefObject<unknown>; | |
listStateRef?: MutableRefObject<ListState<T> | null>; |
Copilot uses AI. Check for mistakes.
id?: string | number; | ||
textValue?: string; | ||
children?: ItemWithKey[]; | ||
[key: string]: unknown; |
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.
[nitpick] The index signature [key: string]: unknown
may be too permissive and could mask type errors. Consider making it optional with [key: string]?: unknown
or being more specific about the expected additional properties.
[key: string]: unknown; | |
[key: string]?: unknown; |
Copilot uses AI. Check for mistakes.
const cloneWithNormalizedKey = (item: ReactElement) => | ||
cloneElement(item, { | ||
key: normalizeKeyValue(item.key), | ||
key: item.key ? normalizeKeyValue(item.key) : undefined, | ||
}); | ||
|
||
const selected: ReactNode[] = []; | ||
const unselected: ReactNode[] = []; | ||
|
||
childrenArray.forEach((child: any) => { | ||
childrenArray.forEach((child: ReactNode) => { |
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.
Setting key
to undefined
in cloneElement
may cause React warnings or issues. React keys should be strings or numbers, not undefined. Consider using the original key or a fallback value.
Copilot uses AI. Check for mistakes.
const item = obj as ItemWithKey; | ||
if (obj && Array.isArray(item.children)) { | ||
// Section-like object – keep order, but sort its children | ||
const sortedChildren = sortArray(item.children); |
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.
The sortArray
function is called recursively for nested children without checking if sorting is actually needed. Consider adding a check to avoid unnecessary sorting when no items are selected.
const sortedChildren = sortArray(item.children); | |
const sortedChildren = | |
selectedSet.size > 0 ? sortArray(item.children) : item.children; |
Copilot uses AI. Check for mistakes.
No description provided.