Skip to content

add keyboard nav to Search - #435

Merged
richardiphillips merged 3 commits into
mainfrom
searchkeyboard1
Sep 29, 2026
Merged

richardiphillips merged 3 commits into
mainfrom
searchkeyboard1

Conversation

@richardiphillips

Copy link
Copy Markdown
Contributor

No description provided.

@richardiphillips
richardiphillips requested review from a team and a balanced review from Copilot September 29, 2026 11:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

Inline dismissal can become permanent, and modal ArrowUp handling conflicts with focus trapping.

Review effort: Balanced
Findings: 2 Medium severity

Open (2)
What changed in this PR

Adds keyboard navigation and dismissal behavior to Search results.

Changes:

  • Adds arrow-key navigation, Escape dismissal, and focus management.
  • Adds Storybook examples and Jest coverage.
  • Bumps the package to 34.3.0 with changelog entry.
File Description
src/​components/​Search.js Implements keyboard and focus behavior.
src/​components/​Search.stories.js Demonstrates inline and modal navigation.
src/​components/​__tests__/​Search.test.js Tests keyboard interactions.
package.json Bumps the package version.
CHANGELOG.md Documents the feature.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/components/Search.js
Comment thread src/components/Search.js
Copilot AI review requested due to automatic review settings September 29, 2026 12:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Async result focus and semantic list accessibility issues remain unresolved.

Review effort: Balanced
Findings: None

Resolved since last review (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Focus results when asynchronously populated after dialog opens

src/​components/​Search.js:66

The focus effect does not react when results arrive while loading remains at its default false. In that supported flow, the dialog opens with an empty list, this timer is a no-op, and a later searchResults update never moves focus to a result. Include a result-readiness signal (for example the rendered result count) in the effect dependencies, while guarding against resetting focus when it is already inside the list, and cover the empty-to-populated async case in the test.

Medium severity Preserve list item semantics around each result button

src/​components/​Search.js:147

Replacing ListItem with ListItemButton removes the semantic list item for each result: ListItemButton supplies button behavior, while the surrounding Box is also non-list markup, so the List no longer exposes its results as list items (the divider is now the only li). Keep a ListItem wrapper and nest ListItemButton inside it, or restructure the map so each result has both valid list and button semantics.

@richardiphillips
richardiphillips merged commit 7f6b453 into main Sep 29, 2026
1 check passed
@richardiphillips
richardiphillips deleted the searchkeyboard1 branch September 29, 2026 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants