-
Notifications
You must be signed in to change notification settings - Fork 3
refactor: Search Box #344
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: Search Box #344
Conversation
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.
Pull Request Overview
Refactors the search feature to use the native <dialog> element and a React context, replaces Downshift with simpler event handling, and switches to next/link routing.
- Removed the Downshift-based search component and its dependency.
- Implemented a
SearchProvidercontext and refactored search button, modal, and results. - Added a dialog-closedby polyfill and standardized routing via
next/link.
Reviewed Changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/booleans.js | Added isBrowser helper |
| src/pages/_document.jsx | Injected dialog-closedby polyfill script |
| src/pages/_app.jsx | Wrapped application in SearchProvider |
| src/components/search/state.jsx | Created search context and provider |
| src/components/search/search.jsx | New search button with platform-specific shortcut |
| src/components/search/search-results.jsx | Wrapped search results rendering |
| src/components/search/search-results-list.jsx | Validated and rendered individual search items |
| src/components/search/search-box.jsx | Core search modal using <dialog> and debounced API calls |
| src/components/search-bar.jsx | Removed old Downshift-based implementation |
| src/components/primary-nav.jsx | Simplified PrimaryMenu props and removed toggle |
| src/components/layout.jsx | Added <SearchBox> to layout |
| src/components/header.jsx | Swapped out old SearchBar for new Search |
| package.json | Removed downshift dependency |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (2)
src/components/layout.jsx:4
- [nitpick] The default export in
search-box.jsxis namedSearchBar, but you're importing it asSearchBox. Consider renaming the component or the import to keep names consistent.
import SearchBox from "./search/search-box";
src/components/primary-nav.jsx:34
- The
setIsMenuOpenprop was removed, but the toggle logic for opening the menu is still needed. Without anonClickhandler on the popover button, the menu cannot open—reintroduce or refactor this toggle.
export default function PrimaryMenu({ className }) {
Co-authored-by: Copilot <[email protected]>
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
Co-authored-by: Copilot <[email protected]>
…org into semantic-search-dialog
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
@kellenmace @moonmeister We should keep the search box pane toward the top of the screen. It also is jumpy on mobile mode. |
…ox from jumping around
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
Fran-A-Dev
left a comment
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.
LGTM
<dialog />next/linkinstead of routing manually