Add support for removable filters #44
Conversation
- Update AppliedFilters to have a `removable` field. If true, applied static filters and facets will be removable and will trigger a new search when removed. - CSS styling for removable filters - TODO: add jest tests J=SLAP-1641 TEST=manual spin up sample app, search 'virginia', and click on static filter and facet options. See that when removable option is true, the applied filter labels have 'X' button. When click, see that the corresponding label is removed and the result is properly updated
|
|
||
| const onRemoveStaticFilterOption = () => { | ||
| document.getElementById(`${filter.filter.fieldId + "_" + filter.filter.value}`)?.click(); |
There was a problem hiding this comment.
can we make a state change that removes the filter, instead of manually clicking the checkbox
There was a problem hiding this comment.
StaticFilter component track its own state, which is different structure from statefulcore FilterState, then update statefulcore's filter state after restructuring it to the Filter|CombinedFilter format, which makes it a bit tricky. We could update answers-headless to have function(s) that traverse down CombinedFilter and remove that filter through a dispatch call, and update StaticFilter component to subscribe to FilterState.static changes. I briefly talked to Tom about this issue and I believe we decided to not make any changes in answers-headless. Is there a specific idea you are thinking of?
There was a problem hiding this comment.
I think for now passing a callback down from the StaticFilters component would be a little cleaner? To put it bluntly I think the current StaticFilters component needs to be written. You currently cannot have multiple static filters on the page, and this is definitely not going to work with FilterSearch
There was a problem hiding this comment.
yeah, this is assuming we only support one StaticFilter since StaticFilter right now directly set statefulcore's FilterState to its own state managed in the component, so the click here would work. I will take a look into passing a callback of some sort
There was a problem hiding this comment.
Update: the option handling static filter selection uses this which requires some binding. I tried using the parent component VerticalSearchPage to get reference of StaticFilter or its handleOptionSelection function that bind this to the instance, and pass to AppliedFilter. Went through different approaches with createRef/useRef and setting callback function through props, nothing really works. We can try messing with setState in verticalSearchPage to manage changes between the two child components. But I think it might be best to wait till the refactor of the StaticFilter to utilize the filter action from answers-headless
There was a problem hiding this comment.
this SGTM, and then we can update to use the new filter actions from answers-headless
| const { fieldId, matcher, value } = filter.filter; | ||
| if (isNearFilterValue(value)) { | ||
| throw Error('unrecognized value type for facet option.'); |
There was a problem hiding this comment.
Do you think it might be preferable to use console.error and returning rather than throw Error because throwing an Error can crash the application
There was a problem hiding this comment.
Also, would a message such as 'A Filter with a NearFilterValue is not a supported RemovableFilter' be more accurate?
There was a problem hiding this comment.
that works too, I will update. I was thinking this would be more immediate in letting user know to resolve this problem, yeah that might not be ideal.
…ess-react into dev/removable-filters
removablefilters Applied static filters and facets will be removable and will trigger a new search when removed.J=SLAP-1641
TEST=manual
spin up sample app, search 'virginia', and click on static filter and facet options. See that the applied filter labels have 'X' button. When click, see that the corresponding label is removed and the result is properly updated
see that new jest tests passed locally