-
Notifications
You must be signed in to change notification settings - Fork 2
[CSL-2954] useFilter Hook Implementation #66
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
[CSL-2954] useFilter Hook Implementation #66
Conversation
CSL-2954 [MVP] Create a useFilters Hook (Only Search)
Business Outcome:
Definition of done:
Additional notes:
|
src/hooks/useFilter.ts
Outdated
| const applyFilter = (facetGroupName: string, facetValue: any) => { | ||
| const newFilters = requestFilters || {}; | ||
|
|
||
| newFilters[facetGroupName] = facetValue; |
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.
Would this apply multiple values properly or would it override the existing one?
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.
It would not, but I was thinking of having whatever is calling the applyFilter function take care of that (i.e. passing a range, array of values, etc). wdyt?
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 okay but I feel like we need to be more explicit with the name of the function and documentation. It might be just me but applyFilter makes me think this will append filters rather than replacing them. Maybe something like setFilter? Doesn't have to be set if we can come up with something better. Also side note now that I'm a Typescript nerd, is the facetValue really any or can we make it more strict?
esezen
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! One small comment about a type now that I am a TS nerd 🤓
src/hooks/useFilter.ts
Outdated
| setRequestConfigs, | ||
| } = useRequestConfigs(); | ||
|
|
||
| const setFilter = (filterName: string, filterValue: any) => { |
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 we can also change this any to PlpFilterValue, right?
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.
yessir, thanks for catching that
Pull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?