-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor(ui): add key action support for dialog navigation #674
Conversation
WalkthroughThe changes involve significant modifications to two components: Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
✅ Deploy Preview for reearth-flow ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
ui/src/components/ActionItem/index.tsx (1)
18-33
: LGTM! Clean forwardRef implementation.The conversion to forwardRef is well-implemented and necessary for the keyboard navigation feature. Consider adding a display name for better debugging:
const ActionItem = forwardRef<HTMLDivElement, Props>( ({ /* props */ }, ref) => { /* implementation */ } ); +ActionItem.displayName = "ActionItem";
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (1)
48-59
: Consider optimizing scroll behavior and state management.A few suggestions to improve the implementation:
- Consider using a more sophisticated scroll behavior
- Selected state could be derived instead of stored
-const [selected, setSelected] = useState<string | undefined>(undefined); +const selected = actions?.[selectedIndex]?.name; useEffect(() => { if (actions?.length) { - setSelected(actions[selectedIndex]?.name); const selectedItem = itemRefs.current[selectedIndex]; if (selectedItem && containerRef.current) { + const container = containerRef.current; + const itemTop = selectedItem.offsetTop; + const itemBottom = itemTop + selectedItem.offsetHeight; + const containerTop = container.scrollTop; + const containerBottom = containerTop + container.offsetHeight; + + if (itemTop < containerTop || itemBottom > containerBottom) { selectedItem.scrollIntoView({ behavior: "smooth", - block: "nearest", + block: itemTop < containerTop ? "start" : "end", }); + } } } }, [selectedIndex, actions]);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
ui/src/components/ActionItem/index.tsx
(2 hunks)ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
[error] 159-159: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
ref={(el) => (itemRefs.current[idx] = el)} | ||
className={"m-1"} |
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.
🛠️ Refactor suggestion
Refactor ref assignment to follow best practices.
The current ref assignment in the expression could be confusing and harder to maintain.
-ref={(el) => (itemRefs.current[idx] = el)}
+ref={el => {
+ if (el) {
+ itemRefs.current[idx] = el;
+ }
+}}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
ref={(el) => (itemRefs.current[idx] = el)} | |
className={"m-1"} | |
ref={el => { | |
if (el) { | |
itemRefs.current[idx] = el; | |
} | |
}} | |
className={"m-1"} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 159-159: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
const [selectedIndex, setSelectedIndex] = useState(0); | ||
const containerRef = useRef<HTMLDivElement>(null); | ||
const itemRefs = useRef<(HTMLDivElement | null)[]>([]); |
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.
🛠️ Refactor suggestion
Consider cleaning up itemRefs when actions change.
The itemRefs array should be reset when actions change to prevent stale references.
useEffect(() => {
if (rawActions && openedActionType?.nodeType) {
+ itemRefs.current = [];
setActions(rawActions?.byType[openedActionType.nodeType]);
}
}, [rawActions, openedActionType.nodeType]);
Committable suggestion skipped: line range outside the PR's diff.
const handleKeyDown = useCallback( | ||
(e: KeyboardEvent) => { | ||
if (e.key === "Enter") { | ||
handleDoubleClick(selected); | ||
} else if (e.key === "ArrowUp") { | ||
setSelectedIndex((prevIndex) => | ||
prevIndex === 0 ? prevIndex : prevIndex - 1, | ||
); | ||
} else if (e.key === "ArrowDown") { | ||
setSelectedIndex((prevIndex) => | ||
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1, | ||
); | ||
} | ||
}, | ||
[handleDoubleClick, selected, actions], | ||
); | ||
|
||
useEffect(() => { | ||
window.addEventListener("keydown", handleKeyDown); | ||
return () => { | ||
window.removeEventListener("keydown", handleKeyDown); | ||
}; | ||
}, [actions, selected, handleKeyDown]); |
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.
Improve keyboard event handling.
Current implementation has several potential issues:
- Global keyboard events could interfere with other inputs
- Missing edge case handling
- Arrow keys might scroll the page
const handleKeyDown = useCallback(
(e: KeyboardEvent) => {
+ // Only handle events when dialog is open
+ if (!openedActionType) return;
+
+ // Ignore events from input fields
+ if (e.target instanceof HTMLInputElement) return;
+
if (e.key === "Enter") {
+ e.preventDefault();
handleDoubleClick(selected);
} else if (e.key === "ArrowUp") {
+ e.preventDefault();
setSelectedIndex((prevIndex) =>
prevIndex === 0 ? prevIndex : prevIndex - 1,
);
} else if (e.key === "ArrowDown") {
+ e.preventDefault();
setSelectedIndex((prevIndex) =>
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1,
);
}
},
- [handleDoubleClick, selected, actions],
+ [handleDoubleClick, selected, actions, openedActionType],
);
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const handleKeyDown = useCallback( | |
(e: KeyboardEvent) => { | |
if (e.key === "Enter") { | |
handleDoubleClick(selected); | |
} else if (e.key === "ArrowUp") { | |
setSelectedIndex((prevIndex) => | |
prevIndex === 0 ? prevIndex : prevIndex - 1, | |
); | |
} else if (e.key === "ArrowDown") { | |
setSelectedIndex((prevIndex) => | |
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1, | |
); | |
} | |
}, | |
[handleDoubleClick, selected, actions], | |
); | |
useEffect(() => { | |
window.addEventListener("keydown", handleKeyDown); | |
return () => { | |
window.removeEventListener("keydown", handleKeyDown); | |
}; | |
}, [actions, selected, handleKeyDown]); | |
const handleKeyDown = useCallback( | |
(e: KeyboardEvent) => { | |
// Only handle events when dialog is open | |
if (!openedActionType) return; | |
// Ignore events from input fields | |
if (e.target instanceof HTMLInputElement) return; | |
if (e.key === "Enter") { | |
e.preventDefault(); | |
handleDoubleClick(selected); | |
} else if (e.key === "ArrowUp") { | |
e.preventDefault(); | |
setSelectedIndex((prevIndex) => | |
prevIndex === 0 ? prevIndex : prevIndex - 1, | |
); | |
} else if (e.key === "ArrowDown") { | |
e.preventDefault(); | |
setSelectedIndex((prevIndex) => | |
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1, | |
); | |
} | |
}, | |
[handleDoubleClick, selected, actions, openedActionType], | |
); | |
useEffect(() => { | |
window.addEventListener("keydown", handleKeyDown); | |
return () => { | |
window.removeEventListener("keydown", handleKeyDown); | |
}; | |
}, [actions, selected, handleKeyDown]); |
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.
@billcookie This looks like a good addition
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (2)
48-59
: Optimize the scroll effect implementation.Consider these improvements for better robustness:
- Add null check for actions[selectedIndex]
- Use optional chaining
useEffect(() => { - if (actions?.length) { + if (actions?.length && actions[selectedIndex]?.name) { setSelected(actions[selectedIndex]?.name); const selectedItem = itemRefs.current[selectedIndex]; if (selectedItem && containerRef.current) { selectedItem.scrollIntoView({ behavior: "smooth", block: "nearest", }); } } }, [selectedIndex, actions]);
Line range hint
158-170
: Enhance accessibility support.Consider adding these accessibility improvements:
- Add
role="listbox"
to the container- Add
role="option"
andaria-selected
to each ActionItem- Add
aria-activedescendant
to indicate the currently selected option-<div ref={containerRef} className="max-h-[50vh] overflow-scroll"> +<div + ref={containerRef} + className="max-h-[50vh] overflow-scroll" + role="listbox" + aria-activedescendant={selected ? `action-${selected}` : undefined} +> {actions?.map((action, idx) => ( <Fragment key={action.name}> <ActionItem ref={(el) => (itemRefs.current[idx] = el)} className={"m-1"} + id={`action-${action.name}`} + role="option" + aria-selected={selected === action.name} action={action} selected={selected === action.name} onSingleClick={handleSingleClick} onDoubleClick={handleDoubleClick} />🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
[error] 162-162: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (3)
37-39
: Consider cleaning up itemRefs when actions change.
The itemRefs array should be reset when actions change to prevent stale references.
121-146
: Improve keyboard event handling.
Current implementation has several potential issues:
- Global keyboard events could interfere with other inputs
- Missing edge case handling
- Arrow keys might scroll the page
162-163
: Refactor ref assignment to follow best practices.
The current ref assignment in the expression could be confusing and harder to maintain.
🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
Show resolved
Hide resolved
…kerDialog/index.tsx Will commit the suggestion as it removes redundant calls for e.preventDefault Co-authored-by: KaWaite <34051327+KaWaite@users.noreply.github.com>
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (1)
Based on the results, I can now generate the final response since we've found that:
- Testing utilities are available in the project (from earlier results showing testing-library in package.json)
- There are no existing tests for the NodePickerDialog component
- The project has keyboard interaction implementations in other components (useShortcuts.ts)
Add tests for NodePickerDialog keyboard navigation and accessibility
The NodePickerDialog component lacks test coverage for its keyboard navigation implementation. Create a new test file with the following coverage:
- Keyboard navigation (Arrow Up/Down) for action selection
- Enter key handling for action confirmation
- Focus management between search input and action items
- Screen reader accessibility for the dialog content
- Proper cleanup of keyboard event listeners
🔗 Analysis chain
Line range hint
121-161
: Add comprehensive testing for keyboard navigation.Please ensure thorough testing of the keyboard navigation implementation:
- Test keyboard navigation with screen readers
- Verify focus management
- Test interaction with search input
- Validate scroll behavior
Let me gather more specific information about the testing setup and existing keyboard navigation tests.
Let me try one more search with corrected path patterns to find test files and testing utilities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing test files fd -e test.tsx -e spec.tsx NodePickerDialog # Check for accessibility testing utilities rg -l "testing-library/jest-dom|@testing-library/react" # Look for similar keyboard navigation implementations ast-grep --pattern 'useEffect($_) { window.addEventListener("keydown", $_); return () => { window.removeEventListener("keydown", $_); }; }'Length of output: 585
Script:
#!/bin/bash # Check for existing keyboard navigation tests rg -l "keyboard|keydown|keypress" "ui/src/**/*.test.tsx" "ui/src/**/*.spec.tsx" # Look for testing utilities specifically in the Editor feature fd . ui/src/features/Editor -t f -e test.tsx -e spec.tsx # Check testing setup and utilities rg "@testing-library/user-event|fireEvent" "ui/src/**/*.test.tsx" "ui/src/**/*.spec.tsx" # Search for accessibility-related testing patterns rg "role=|aria-|screen.getBy" "ui/src/**/*.test.tsx" "ui/src/**/*.spec.tsx"Length of output: 675
Script:
#!/bin/bash # Look for test files in the entire ui directory fd -e test.tsx -e spec.tsx . ui/src # Check for keyboard event testing patterns in all test files rg -l "keyboard|keydown|keypress" $(fd -e test.tsx -e spec.tsx . ui/src) # Check for testing-library usage patterns rg "fireEvent|userEvent|screen\." $(fd -e test.tsx -e spec.tsx . ui/src) # Look specifically for Dialog component tests rg -l "Dialog.*test" ui/srcLength of output: 716
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
[error] 160-160: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (3)
37-39
: Consider cleaning up itemRefs when actions change.
The itemRefs array should be reset when actions change to prevent stale references.
160-161
: Refactor ref assignment to follow best practices.
The current ref assignment in the expression could be confusing and harder to maintain.
🧰 Tools
🪛 Biome (1.9.4)
[error] 160-160: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
121-144
:
Improve keyboard event handling and accessibility.
The current keyboard event handling has several issues:
- Global event listener could interfere with other inputs
- Missing focus management
- Lacks proper accessibility support
+const [isFocused, setIsFocused] = useState(false);
+const lastInteractionWasKeyboard = useRef(false);
const handleKeyDown = useCallback(
(e: KeyboardEvent) => {
+ // Only handle events when dialog is focused
+ if (!isFocused) return;
+
+ // Ignore events from input fields
+ if (e.target instanceof HTMLInputElement) return;
+
+ lastInteractionWasKeyboard.current = true;
e.preventDefault();
if (e.key === "Enter") {
handleDoubleClick(selected);
} else if (e.key === "ArrowUp") {
setSelectedIndex((prevIndex) =>
prevIndex === 0 ? prevIndex : prevIndex - 1,
);
} else if (e.key === "ArrowDown") {
setSelectedIndex((prevIndex) =>
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1,
);
}
},
- [handleDoubleClick, selected, actions],
+ [handleDoubleClick, selected, actions, isFocused],
);
-useEffect(() => {
- window.addEventListener("keydown", handleKeyDown);
- return () => {
- window.removeEventListener("keydown", handleKeyDown);
- };
-}, [actions, selected, handleKeyDown]);
+// Add role and aria attributes to the container
+<div
+ ref={containerRef}
+ className="max-h-[50vh] overflow-scroll"
+ role="listbox"
+ tabIndex={0}
+ aria-activedescendant={selected}
+ onFocus={() => setIsFocused(true)}
+ onBlur={() => setIsFocused(false)}
+ onKeyDown={handleKeyDown}
+>
Likely invalid or redundant comment.
useEffect(() => { | ||
if (actions?.length) { | ||
setSelected(actions[selectedIndex]?.name); | ||
const selectedItem = itemRefs.current[selectedIndex]; | ||
if (selectedItem && containerRef.current) { | ||
selectedItem.scrollIntoView({ | ||
behavior: "smooth", | ||
block: "nearest", | ||
}); | ||
} | ||
} | ||
}, [selectedIndex, actions]); |
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.
🛠️ Refactor suggestion
Add safety checks and optimize scroll behavior.
The current implementation could be improved for better reliability and user experience:
- Add null/empty checks before accessing array index
- Prevent unnecessary scrolling on initial render
useEffect(() => {
- if (actions?.length) {
+ if (actions?.length && actions[selectedIndex]) {
setSelected(actions[selectedIndex]?.name);
const selectedItem = itemRefs.current[selectedIndex];
- if (selectedItem && containerRef.current) {
+ // Only scroll if selection changed by keyboard navigation
+ if (selectedItem && containerRef.current && lastInteractionWasKeyboard.current) {
selectedItem.scrollIntoView({
behavior: "smooth",
block: "nearest",
});
}
}
}, [selectedIndex, actions]);
Committable suggestion skipped: line range outside the PR's diff.
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.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (1)
Line range hint
1-177
: Consider extracting keyboard navigation logic into a custom hookThe component is handling multiple responsibilities. Consider extracting the keyboard navigation logic into a reusable custom hook for better maintainability and reusability:
function useKeyboardNavigation<T>({ items, onSelect, onConfirm, isActive, }: { items: T[]; onSelect: (index: number) => void; onConfirm: () => void; isActive: boolean; }) { const [selectedIndex, setSelectedIndex] = useState(0); const handleKeyDown = useCallback( (e: KeyboardEvent) => { if (!isActive) return; if (e.key === "Enter") { e.preventDefault(); onConfirm(); } else if (e.key === "ArrowUp") { e.preventDefault(); setSelectedIndex((prev) => (prev === 0 ? prev : prev - 1)); } else if (e.key === "ArrowDown") { e.preventDefault(); setSelectedIndex((prev) => prev === items.length - 1 ? prev : prev + 1 ); } }, [items, isActive, onConfirm] ); useEffect(() => { onSelect(selectedIndex); }, [selectedIndex, onSelect]); useEffect(() => { window.addEventListener("keydown", handleKeyDown); return () => window.removeEventListener("keydown", handleKeyDown); }, [handleKeyDown]); return selectedIndex; }Would you like me to help create this custom hook in a separate file?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
(6 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx
[error] 162-162: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (3)
ui/src/features/Editor/components/OverlayUI/components/NodePickerDialog/index.tsx (3)
37-39
: Clean up itemRefs when actions change
The itemRefs array should be reset when actions change to prevent stale references.
48-59
: Add safety checks and optimize scroll behavior
The current implementation could be improved for better reliability and user experience.
162-163
: Refactor ref assignment to follow best practices
The current ref assignment in the expression could be confusing and harder to maintain.
🧰 Tools
🪛 Biome (1.9.4)
[error] 162-162: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
const handleKeyDown = useCallback( | ||
(e: KeyboardEvent) => { | ||
if (e.key === "Enter") { | ||
e.preventDefault(); | ||
handleDoubleClick(selected); | ||
} else if (e.key === "ArrowUp") { | ||
e.preventDefault(); | ||
setSelectedIndex((prevIndex) => | ||
prevIndex === 0 ? prevIndex : prevIndex - 1, | ||
); | ||
} else if (e.key === "ArrowDown") { | ||
e.preventDefault(); | ||
setSelectedIndex((prevIndex) => | ||
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1, | ||
); | ||
} | ||
}, | ||
[handleDoubleClick, selected, actions], | ||
); | ||
|
||
useEffect(() => { | ||
window.addEventListener("keydown", handleKeyDown); | ||
return () => { | ||
window.removeEventListener("keydown", handleKeyDown); | ||
}; | ||
}, [actions, selected, handleKeyDown]); |
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.
🛠️ Refactor suggestion
Implement focus management for better accessibility
The keyboard navigation implementation needs focus management to ensure proper accessibility:
- The dialog should trap focus within its bounds
- Selected items should receive focus for screen readers
- ESC key handling is missing for dialog dismissal
+ const [hasFocus, setHasFocus] = useState(false);
const handleKeyDown = useCallback(
(e: KeyboardEvent) => {
+ // Only handle events when dialog has focus
+ if (!hasFocus) return;
+
if (e.key === "Enter") {
e.preventDefault();
handleDoubleClick(selected);
+ } else if (e.key === "Escape") {
+ e.preventDefault();
+ onClose();
} else if (e.key === "ArrowUp") {
e.preventDefault();
setSelectedIndex((prevIndex) =>
prevIndex === 0 ? prevIndex : prevIndex - 1,
);
} else if (e.key === "ArrowDown") {
e.preventDefault();
setSelectedIndex((prevIndex) =>
prevIndex === (actions?.length || 1) - 1 ? prevIndex : prevIndex + 1,
);
}
},
- [handleDoubleClick, selected, actions],
+ [handleDoubleClick, selected, actions, hasFocus, onClose],
);
useEffect(() => {
window.addEventListener("keydown", handleKeyDown);
return () => {
window.removeEventListener("keydown", handleKeyDown);
};
- }, [actions, selected, handleKeyDown]);
+ }, [handleKeyDown]);
Committable suggestion skipped: line range outside the PR's diff.
Overview
Allow users to be able to navigate using arrows keys and "double click" with enter.
What I've done
Added key support for navigating the menu with up, down and enter keys.
What I haven't done
How I tested
manually
Screenshot
Which point I want you to review particularly
Memo
Summary by CodeRabbit
Release Notes
New Features
ActionItem
component to support direct DOM referencing viaref
.NodePickerDialog
with keyboard navigation for selecting actions and automatic scrolling to the selected item.Bug Fixes
NodePickerDialog
scrolls into view when the selection changes.Refactor