Refactor Tree component with RovingFocusGroup #982
Conversation
WalkthroughThis update removes the older context-based batch navigation and focus management from the Tree components. The Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant TR as Tree.Root
participant RG as RovingFocusGroup
participant TI as Tree.Item
U->>TR: Initiates focus or key event
TR->>RG: Delegates event handling
RG->>TI: Updates focus based on event
TI-->>RG: Acknowledges focus change
RG-->>TR: Returns updated focus state
Possibly related issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/ui/Tree/fragments/TreeRoot.tsx (2)
15-15: Potentially unused referenceThe
treeRefis created but might not be serving a purpose now that batch navigation has been removed. Consider whether this ref is still needed in the current implementation.
17-27: Good refactoring to RovingFocusGroup for focus managementThe implementation appropriately delegates focus management to the RovingFocusGroup components, which is a more maintainable approach than the previous custom implementation using context,
moveUp, andmoveDownfunctions.However, the TreeContext is now providing an empty object. If no context values are being used by consumers, consider removing the context provider entirely.
src/components/ui/Tree/fragments/TreeItem.tsx (3)
1-1: Remove unused importThe
useRefimport is defined but never used, as indicated by the static analysis tool. This should be removed to keep the imports clean.-import React, { useId, useRef, useState } from 'react'; +import React, { useId, useState } from 'react';🧰 Tools
🪛 GitHub Check: lint
[warning] 1-1:
'useRef' is defined but never used
24-40: Good implementation of RovingFocusGroup.ItemWrapping ButtonPrimitive with RovingFocusGroup.Item correctly integrates the component with the new focus management system.
However, there are several inline styles that could be extracted into a separate variable for better readability and maintainability:
+const buttonStyles = { + display: 'block', + alignItems: 'center', + gap: '0.5rem' +}; + +const contentStyles = (level: number) => ({ + display: 'flex', + alignItems: 'center', + gap: '0.5rem', + paddingLeft: `${level * 16}px` +}); return <> <RovingFocusGroup.Item > <ButtonPrimitive className={clsx(className)} onClick={handleClick} - style={{ display: 'block', alignItems: 'center', gap: '0.5rem' }} + style={buttonStyles} {...props}> - <div style={{ display: 'flex', alignItems: 'center', gap: '0.5rem', paddingLeft: `${level * 16}px` }}> + <div style={contentStyles(level)}> <div> {isToggled ? 'v' : '>'} </div>
31-37: Consider improving the toggle indicatorsThe current toggle indicators ('>' and 'v') are simplistic. Consider using more intuitive icons or symbols for expanding/collapsing tree items, or even better, using proper chevron icons from an icon library that's already in the project.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/ui/Tree/fragments/TreeItem.tsx(2 hunks)src/components/ui/Tree/fragments/TreeRoot.tsx(2 hunks)src/components/ui/Tree/stories/Tree.stories.tsx(1 hunks)
🧰 Additional context used
🪛 GitHub Check: lint
src/components/ui/Tree/fragments/TreeItem.tsx
[warning] 1-1:
'useRef' is defined but never used
🔇 Additional comments (4)
src/components/ui/Tree/fragments/TreeRoot.tsx (1)
4-6: Good selection of imports to implement the RovingFocusGroup patternThe addition of RovingFocusGroup and Primitive imports aligns with the PR objective of refactoring the Tree component's focus management system.
src/components/ui/Tree/stories/Tree.stories.tsx (2)
7-46: Good extraction of items into a separate constantMoving the items array to a constant at the file level improves readability and organization. This makes the tree structure more explicit and easier to maintain.
47-58: Better component structure with explicit Tree.Root and Tree.ItemThe new structure with explicit Tree.Root and Tree.Item components provides a clearer example of how to use the refactored Tree component with RovingFocusGroup.
src/components/ui/Tree/fragments/TreeItem.tsx (1)
6-6: Good addition of RovingFocusGroup importThe RovingFocusGroup import correctly aligns with the PR objective of refactoring the Tree component with improved focus management.
Summary by CodeRabbit