feat: filter pills #422
feat: filter pills #422
Conversation
marcellmueller
force-pushed
the
feat/filter-pills
branch
2 times, most recently
from
b6294a1 to
fcdbe52
Compare
marcellmueller
force-pushed
the
feat/filter-pills
branch
2 times, most recently
from
b344858 to
d5771ce
Compare
marcellmueller
force-pushed
the
feat/filter-pills
branch
4 times, most recently
from
a083fb8 to
cda6aed
Compare
marcellmueller
force-pushed
the
feat/filter-pills
branch
from
cda6aed to
947df08
Compare
| } else { | ||
| filterChipStore.setState((prevState) => | ||
| prevState.filter((filter) => filter.id !== id), | ||
| ); |
There was a problem hiding this comment.
(nit): When adding a filter to filterChipStore, there's no check to prevent duplicates
if (!filterChipStore.state.some((filter) => filter.id === id)) {
filterChipStore.setState((prevState) => [...prevState, { param, id, label }]);
}
There was a problem hiding this comment.
Nice catch thanks!
| [...searchParams].filter(([key]) => !['filter', 'page'].includes(key)), | ||
| ); | ||
|
|
||
| if (Object.keys(filterParams).length === 0) return; |
There was a problem hiding this comment.
(nit): This is redundant because filters.forEach() will automatically skip iteration if filterParams is empty
There was a problem hiding this comment.
hmm I looked into this and definitely think it's intentional so we don't loop through the whole filter side menu list that comes from the search store. It is iterating through them so we can get filter data like the label and add it to the chip, since we don't have that just from the param on page load.
I tried removing this and my tests failed with this:
AssertionError: expected "spy" to not be called at all, but actually been called 1 times
...
expect(filterChipStore.setState).not.toHaveBeenCalled();
| }); | ||
| }); | ||
|
|
||
| filterChipStore.setState(() => [...filterChipList]); |
There was a problem hiding this comment.
(nit): The function replaces the entire state instead of merging it with existing values - is that intentional? If we want to preserve the existing chips we should do something like this
filterChipStore.setState((prevState) => [...prevState, ...filterChipList]);
There was a problem hiding this comment.
This hook is used when either reloading the page, or when visiting the url with filter params, so there's no current state.
There was a problem hiding this comment.
Ah makes sense. Ignore the suggestion than :) Thank you!
| <div className="filter-chips-container"> | ||
| {filterChips.map((filterChip) => { | ||
| const { id, label, param } = filterChip; | ||
| return <FilterChip key={label} param={param} id={id} label={label} />; |
There was a problem hiding this comment.
(nit): The key for each FilterChip is set to label which might not be unique. can we please use id instead.
#307
searchResultsStore