Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
There was a problem hiding this comment.
Pull request overview
This PR introduces a new TreeView component to the Cauldron design system, enabling users to display and interact with hierarchical tree structures with support for single/multiple selection modes and custom actions.
Changes:
- Added TreeView component with TreeViewItem subcomponent supporting single/multiple selection modes
- Integrated react-aria-components library for accessibility features
- Added CSS styling for tree view with theme support
- Created comprehensive test coverage for TreeView functionality
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/styles/tree-view.css | CSS styles for TreeView component with light/dark theme support |
| packages/styles/index.css | Import statement for tree-view.css |
| packages/react/src/index.ts | Export statement for TreeView component |
| packages/react/src/components/TreeView/index.tsx | Main TreeView component definition |
| packages/react/src/components/TreeView/TreeViewItem.tsx | TreeViewItem subcomponent with selection handling |
| packages/react/src/components/TreeView/TreeView.test.tsx | Test suite for TreeView component |
| packages/react/src/components/Checkbox/index.tsx | Added onChangeToggle prop to Checkbox component |
| packages/react/package.json | Added react-aria-components dependency |
| package.json | Added packageManager field |
| docs/pages/components/TreeView.mdx | Documentation for TreeView component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
It'd be good to go back to design and clarify the expected behavior of the checkboxes - the figma doesn't clarify explicitly, but some of the examples it cites (eg, Fluent 2) use a pattern where selecting a parent would implicitly select all children, and unselecting a child would implicitly set the parent to either an indeterminate or unselected state (depending on if other siblings were still selected). adobe/react-spectrum#6589 suggests that it might be challenging to actually implement that ourselves without patching the spectrum tree view and that we'd likely prefer to wait for adobe/react-spectrum#8800 rather than implement it ourselves, but we should make sure that stakeholders/design are ok with the current behavior in the meantime. |
Good points, Dan. Also a quick technical note for anyone reviewing this: Adobe actually has two component libraries available that we can use - Spectrum and React Aria Components. I ended up choosing React Aria Components since unlike Spectrum, it doesn't make style assumptions and allows us to incorporate our existing Deque theming into new components. |
|
This pull request has been open for 30 days with no activity. If no further activity occurs, it will be closed in 14 days. |
|
This pull request was closed because it has been inactive for 14 days since being marked as stale. |
| import { Tree } from 'react-aria-components'; | ||
| import TreeViewItem from './TreeViewItem'; | ||
|
|
||
| export interface TreeViewFileType { |
There was a problem hiding this comment.
Does this have to be named "file"? Could it be more generic like TreeViewNode?
| :root { | ||
| --treeview-padding: 16px; | ||
| --treeview-focus-ring-color: var(--focus-light); | ||
| --treeview-selected-color: var(--focus-light); | ||
| --treeview-highlight-background: var(--accent-light); | ||
| } | ||
|
|
||
| .cauldron--theme-dark { | ||
| --treeview-focus-ring-color: var(--focus-dark); | ||
| --treeview-selected-color: var(--focus-dark); | ||
| --treeview-highlight-background: var(--accent-dark); | ||
| } |
There was a problem hiding this comment.
The variables should be named --tree-view-* to be consistent with the rest of the codebase.
| <TreeView ariaLabel="Test TreeView" items={items} selectionMode="single" /> | ||
| ); | ||
| const child1 = getByText('TreeView'); | ||
| fireEvent.click(child1); |
There was a problem hiding this comment.
We should prefer userEvent over fireEvent to simulate how a user would interact with this component.
https://testing-library.com/docs/user-event/intro/#differences-from-fireevent
There was a problem hiding this comment.
Can we include tests for things like the handleOnAction workaround and checkbox rendering based on selectionMode?
| <Icon type="chevron-right" /> | ||
| </Button> | ||
| {selectionMode !== 'none' ? ( | ||
| <Checkbox |
There was a problem hiding this comment.
I'm not able to select an individual checkbox with space. As soon as I let go of the space bar, it becomes unchecked again.
checkbox-selection-bug.mov
I'm able to use space on the checkbox on https://react-aria.adobe.com/Tree.
mateoviilla1
left a comment
There was a problem hiding this comment.
Review
Critical
TreeViewItem.tsx:60 — Checkbox Space key bug
Pressing Space on a focused checkbox doesn't maintain the checked state — it resets immediately on keyup. The combination of onChangeToggle={false} on Cauldron's <Checkbox> and react-aria's selection state sync causes the visual state to reset on re-render. For an accessibility-first component this is a blocking issue. Consider using react-aria's own <Checkbox> from react-aria-components, or routing keyboard interaction through TreeItem's selection handler so the state flows through react-aria's unified event model.
TreeViewItem.tsx:30–43 — Direct DOM mutation in handleOnAction
handleOnAction calls setAttribute('aria-selected', ...) directly on the DOM node to work around react-aria not toggling selection on action. This bypasses React's render cycle, causing the virtual DOM and real DOM to diverge — any subsequent re-render will overwrite the mutation. The correct fix is to manage selection externally via selectedKeys/onSelectionChange props on <Tree>, keeping state in React rather than the DOM.
Important
index.tsx:14 — onAction type mismatch
onAction is typed as () => void but handleOnAction calls it as onAction(key) where key is a string. TypeScript silently accepts calling a () => void with arguments, so this compiles but the public API contract is wrong. Change to (key: string) => void.
tree-view.css:1–12 — CSS variable naming
Custom properties use --treeview-* (e.g. --treeview-padding, --treeview-focus-ring-color). The Cauldron codebase convention uses --tree-view-* with word separators. Rename all variables accordingly.
TreeView.test.tsx — Test quality
Three gaps per CONTRIBUTING.md:
fireEventshould be replaced withuserEventfrom@testing-library/user-eventto simulate real user interaction (focus/pointer/keyboard sequences).- Missing
jest-axeassertions — at least oneexpect(await axe(container)).toHaveNoViolations()per rendered variant. - Missing screenshot tests.
index.tsx:5 — TreeViewFileType naming
The exported type is named TreeViewFileType, implying the tree is file-system specific. The component is generic. Rename to TreeViewItem or TreeViewNode.
Suggestions
TreeViewItem.tsx:73 — Redundant items prop on <Collection>
<Collection items={children}> passes items but the children are rendered manually as a JSX array. In react-aria, items is only consumed when children is a render function — here it's silently ignored. Either remove items={children}, or switch to the dynamic collection pattern: <Collection items={children}>{(child) => <TreeViewItem ... />}</Collection>.
TreeView.mdx:4 — Incorrect source link
The frontmatter source field points to TreeView.tsx which doesn't exist — the actual entry point is index.tsx. Update to .../TreeView/index.tsx.
Closes:
https://github.com/dequelabs/axe-reports/issues/2952
#1290