fix: focus the search input through the owning component instead of a DOM query - #3252
Chirag6722 wants to merge 2 commits into
Conversation
… DOM query The '/' shortcut in app.vue and the ArrowUp handler on the search page located the search input with a selector that depended on markup in Header/SearchBox.vue and pages/index.vue. Add a useSearchInputFocus composable: app.vue provides a registry, both search inputs register their focus method, and the shortcut handlers call it. resolves npmx-dev#3216
Thanks for opening this pull request! 🎉We really appreciate you taking the time to contribute, @Chirag6722. A maintainer will take a look as soon as they can. In the meantime, please make sure that:
If anything needs adjusting we'll leave comments here. Thanks again! |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe PR adds a scoped search-input focus context. The homepage and header register focus targets. Keyboard handlers use the context instead of DOM selectors. Unit and end-to-end tests cover focus routing, cleanup, route changes, and the ChangesSearch input focus
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to Search focus routing preserves the navigation fallback when no target is available and includes lifecycle and shortcut coverage. The change is ready to merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
graphieros
left a comment
graphieros
left a comment
There was a problem hiding this comment.
@Chirag6722
As you know, if you read our contribution guide, the use of AI to assist with coding is perfectly acceptable, as long as it is human driven.
However, using your own voice is required when communicating with npmx contributors.
Please consider writing the PR description with your own words. It can be in your native language if it is more practical.
Thank you^^
This branch was successfully deployed
2 participants
resolves #3216
Context
The
/shortcut inapp.vueand the ArrowUp handler inpages/search.vuefound the search input with adocument.querySelector. That input lives in two other components, so a markup change there would silently break both shortcuts.Description
Added a small
useSearchInputFocuscomposable.app.vueprovides it,Header/SearchBox.vueandpages/index.vueregister their own input, and the two handlers call the injectedfocus()instead of querying the DOM. The header box returnsfalsewhen its input is not rendered (homepage), so the homepage input is used there. If nothing is registered,/still falls back torouter.push({ name: 'search' }).Behaviour is unchanged. The existing mobile quirk (header input hidden by CSS, so focus does nothing) is left as is; happy to follow up on it.
Tests: 7 cases in
test/nuxt/composables/use-search-input-focus.spec.tsplus one e2e case for/on the homepage. They fail if I remove the registrations or the cleanup. Lint, fmt, nuxt tests and the search e2e tests pass locally;test:typesI could only verify on CI.