feat(CheckTreePicker): support onCascadeChange prop #4287
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an onCascadeChange callback to CheckTreePicker, allowing consumers to receive leaf-node values when selections cascade.
- Added
onCascadeChangeto the component props and invocation points - Implemented a recursive transformer to extract leaf values and wired it into selection logic
- Expanded test coverage and updated both English and Chinese documentation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/CheckTreePicker/CheckTreePicker.tsx | Added onCascadeChange prop, cascade-change handler, and value-to-leaf transformation |
| src/CheckTreePicker/test/CheckTreePickerSpec.tsx | New tests verifying onCascadeChange behavior |
| docs/pages/components/check-tree-picker/zh-CN/index.md | Added onCascadeChange entry in Chinese docs |
| docs/pages/components/check-tree-picker/en-US/index.md | Added onCascadeChange entry in English docs |
Comments suppressed due to low confidence (3)
src/CheckTreePicker/CheckTreePicker.tsx:220
- [nitpick] The function name
handleTransValue2Childrenis ambiguous. Consider renaming it to something more descriptive liketransformToLeafNodesorgetLeafNodesFromSelection.
const handleTransValue2Children = useEventCallback((nextSelectedNodes: TreeNode[]) => {
src/CheckTreePicker/test/CheckTreePickerSpec.tsx:256
- No test covers the case when the
cascadeprop is explicitly enabled or disabled. Consider adding a test that passescascade={true}to ensure the correct branch ofhandleChangeCascadeis executed.
it('Should call `onCascadeChange` with values of children', () => {
src/CheckTreePicker/CheckTreePicker.tsx:228
- The variables
disabledItemValuesanduncheckableItemValuesare used here but are not explicitly destructured from props, which can lead toundefinedat runtime. Please destructure them frompropsWithDefaultsor reference them viarest.
!disabledItemValues.includes(childValue) &&
myNameIsDu
force-pushed
the
feat/checkTreePicker
branch
from
43efc65 to
3a25f49
Compare
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for the onCascadeChange prop in the CheckTreePicker component. It includes new tests to verify the callback behavior when cascade changes occur, integrates the callback into the component’s change handling logic, and updates both Chinese and English documentation.
- Adds tests for onCascadeChange behavior with different conditions (all children, without disabled, without uncheckable values).
- Integrates onCascadeChange in the CheckTreePicker component, handling both cascade and non-cascade cases.
- Updates documentation for the new prop in both zh-CN and en-US locales.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/CheckTreePicker/test/CheckTreePickerSpec.tsx | Adds tests to verify onCascadeChange behavior under different scenarios. |
| src/CheckTreePicker/CheckTreePicker.tsx | Introduces the onCascadeChange prop and integrates its callback logic. |
| docs/pages/components/check-tree-picker/zh-CN/index.md | Updates documentation with the onCascadeChange prop description. |
| docs/pages/components/check-tree-picker/en-US/index.md | Updates documentation with the onCascadeChange prop description. |
| ) => React.ReactNode; | ||
|
|
||
| /** | ||
| * In the cascade case, the leaf node's value change callbacks |
There was a problem hiding this comment.
The JSDoc comment for onCascadeChange appears truncated; please update it to clearly describe that the callback is triggered when the value of leaf nodes changes in cascade mode.
| return nextSelectedNodes | ||
| .map((node: TreeNode) => { | ||
| const currentNode = node.refKey ? flattenedNodes[node.refKey] : null; | ||
| if (currentNode && currentNode[childrenKey] && currentNode[childrenKey].length) { | ||
| const childNodes = currentNode[childrenKey].filter((child: TreeNode) => { | ||
| const childValue = child[valueKey]; | ||
| return ( | ||
| !disabledItemValues.includes(childValue) && | ||
| !uncheckableItemValues.includes(childValue) | ||
| ); | ||
| }); | ||
| return handleTransValue2Children(childNodes); | ||
| } | ||
| return node; | ||
| }) | ||
| .flat(); |
There was a problem hiding this comment.
[nitpick] Consider refactoring the mapping and flattening logic using Array.flatMap() for improved readability and potential performance benefits.
| return nextSelectedNodes | |
| .map((node: TreeNode) => { | |
| const currentNode = node.refKey ? flattenedNodes[node.refKey] : null; | |
| if (currentNode && currentNode[childrenKey] && currentNode[childrenKey].length) { | |
| const childNodes = currentNode[childrenKey].filter((child: TreeNode) => { | |
| const childValue = child[valueKey]; | |
| return ( | |
| !disabledItemValues.includes(childValue) && | |
| !uncheckableItemValues.includes(childValue) | |
| ); | |
| }); | |
| return handleTransValue2Children(childNodes); | |
| } | |
| return node; | |
| }) | |
| .flat(); | |
| return nextSelectedNodes.flatMap((node: TreeNode) => { | |
| const currentNode = node.refKey ? flattenedNodes[node.refKey] : null; | |
| if (currentNode && currentNode[childrenKey] && currentNode[childrenKey].length) { | |
| const childNodes = currentNode[childrenKey].filter((child: TreeNode) => { | |
| const childValue = child[valueKey]; | |
| return ( | |
| !disabledItemValues.includes(childValue) && | |
| !uncheckableItemValues.includes(childValue) | |
| ); | |
| }); | |
| return handleTransValue2Children(childNodes); | |
| } | |
| return [node]; | |
| }); |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for an onCascadeChange prop to the CheckTreePicker component. It updates the component’s TypeScript interface and implementation to handle cascade change behavior, introduces new tests to verify the callback functionality (including handling disabled and uncheckable values), and updates both Chinese and English documentation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/CheckTreePicker/test/CheckTreePickerSpec.tsx | Added tests to verify that onCascadeChange is called correctly in various scenarios |
| src/CheckTreePicker/CheckTreePicker.tsx | Integrated the onCascadeChange prop into the component’s logic and added helper functions for cascade processing |
| docs/pages/components/check-tree-picker/zh-CN/index.md | Updated to document the new onCascadeChange callback |
| docs/pages/components/check-tree-picker/en-US/index.md | Updated to document the new onCascadeChange callback |
Comments suppressed due to low confidence (1)
src/CheckTreePicker/CheckTreePicker.tsx:82
- Consider updating the comment for 'onCascadeChange' to provide a complete and clear description of its behavior. For example, explain that the callback receives either the direct value or, in cascade mode, an array of leaf node values.
/** In the cascade case, the leaf node's value change callbacks
| /** | ||
| * In the cascade case, the leaf node's value change callbacks | ||
| */ | ||
| onCascadeChange?: (v: ValueType, event: React.SyntheticEvent) => void; |
There was a problem hiding this comment.
Ensure that the type for 'onCascadeChange' is consistent with its documentation. If the callback is intended to always receive an array of values (as indicated in the docs), consider updating the type to reflect that or clarifying the expected behavior when cascade is false.
2 participants
No description provided.