-
Notifications
You must be signed in to change notification settings - Fork 9
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 #1489] Complete search filter accordion logic #1490
[Issue #1489] Complete search filter accordion logic #1490
Conversation
@@ -25,6 +29,7 @@ const FilterCheckbox: React.FC<FilterCheckboxProps> = ({ | |||
onChange={onChange} | |||
disabled={disabled} | |||
checked={checked} | |||
value={value || ""} |
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.
Allow a value to be passed in case we want to explicitly have a value set for checkbox submissions on search
Without this it's just { name: "status-forecased", value: "on"}
initialQueryParams, | ||
queryParamKey, | ||
formRef, | ||
); |
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.
More logic was added to useSearchFilter
since the SearchFilterAccordion
is getting sizeable
onClick={(event) => { | ||
event.preventDefault(); | ||
onClearAll?.(); | ||
}} |
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.
select all
/clear all
options need to prevent form submit. The actual query param update and form submit happens together in useSearchFilter
@@ -1,21 +1,47 @@ | |||
"use client"; | |||
|
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.
The name of this file might be better as useSearchFilterAccordion
(if it's not also being used for opportunity status checkboxes - it would need to be refactored a bit for that) - but no pressing need to change the filename
@@ -1,6 +1,6 @@ | |||
"use client"; | |||
|
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.
Explicitly mark all these files as use client
so its clear
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, but I'd suggest @acouch give the hooks a look.
fundingInstrument: fundingInstrumentQueryParams, | ||
} = requestURLQueryParams; | ||
|
||
// TODO: move this to server-side calculation? |
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.
I think that's right; the frontend shouldn't be responsible for this. It seems like the API should return an error if we request something specific from the API that doesn't exist.
Summary
Fixes #1489
Time to review: 10-15 minutes
Changes proposed
useSearchFormState
that wraps the new ReactuseFormState
use client
Demo
Screen.Recording.2024-03-18.at.7.28.22.AM.mov