-
Notifications
You must be signed in to change notification settings - Fork 22
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
[Issue #2732] clear search input when clicking search nav link #2756
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.
Confirmed it works as expected, left a few thoughts. If this is the approach we wanna take, feel free to merge. I do think this bug may raise bigger questions about state management on the search page since I know there's discussions around how to handle having 'forcasted' and 'posted' checked by default too, so I think we just have some complex state on this particular page and may be worth doing a wholistic eval?
@@ -6,6 +6,7 @@ | |||
"node": ">=20.0.0" | |||
}, | |||
"scripts": { | |||
"all-checks": "npm run test && npm run lint && npm run ts:check && npm run build", |
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.
love this!! kudos!
@@ -36,6 +52,7 @@ export default function SearchBar({ query }: SearchBarProps) { | |||
</label> | |||
<div className="usa-search usa-search--big" role="search"> | |||
<input | |||
ref={inputRef} |
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.
Non blocking: Is it worth just adding either useState or possibly the newer server action state management for this form as opposed to using refs and essentially handling setting state manually?
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.
Approving, per Emily's review
Note that @emilycnava and I discussed offline, both agree that this solution doesn't feel great but that we don't have a better idea without creating some global state or other relatively heavy solution that would be out of scope for this ticket. We should review this with whoever is interested and figure out what to do with this sort of thing as a next step. |
Could we use |
@andycochran I'll look into that, if we're ok with doing a full page refresh in this case |
return [ | ||
{ text: t("nav_link_home"), href: "/" }, | ||
getSearchLink(path.includes("/search")), | ||
{ text: t("nav_link_process"), href: "/process" }, |
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.
Didn't get a chance to review this, but this could be simplified by just including an array of links instead of an array of objects that is then mapped to links. Something for future clean-up.
Summary
Fixes #2732
Time to review: 10 mins
Changes proposed
Fixes a bug where the search bar does not clear when clicking "search" on the nav while on the search page.
The solution I came up with was to append a query param to the search nav item when on the search page, whose presence is a signal to the page to clear the search bar.
The approach is not ideal, but a fix requires knowing when a navigation has occurred, and Next does not provide a simple way to know that without relying on something like global state.
Context for reviewers
Test steps
Additional information