feat: only allow one filter open at a time, easy closing #3084
Conversation
* refactor: moar abstraction * tiny nits, update tests * fix test --------- Co-authored-by: angelathe <[email protected]>
Co-authored-by: Mary McGrath <[email protected]>
Co-authored-by: [email protected]
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3084 +/- ##
==========================================
+ Coverage 86.75% 88.41% +1.66%
==========================================
Files 217 80 -137
Lines 13527 4126 -9401
Branches 690 696 +6
==========================================
- Hits 11735 3648 -8087
+ Misses 1770 456 -1314
Partials 22 22
Flags with carried forward coverage won't be shown. Click here to find out more.
|
mcmcgrath13
requested review from
akasper,
angelathe,
austin-hall-skylight,
BobanL,
gordonfarrell,
JNygaard-Skylight,
lina-roth and
robertmitchellv
as code owners
|
|
|
|
||
| // When a filter is open, close it if the escape key is hit or a click happens | ||
| // outside the <Filter /> component (implemented by stopping click propogation on <Filter />) | ||
| React.useEffect(() => { |
There was a problem hiding this comment.
Tiny nit, but can we remove the React. here for consistency? We already import and use useEffect for other consts in this file
| if (filterBoxOpen) { | ||
| window.addEventListener("keydown", handleEscapeFilters); | ||
| window.addEventListener("click", resetFilters); | ||
| return () => { |
There was a problem hiding this comment.
Does this cleanup only happen if filterBoxOpen === null?
Also, totally cool, I'd never seen useEffect cleanup functions be used before 🙌
| @@ -133,7 +153,7 @@ export const Filter = ({ | |||
| onSubmit={(e) => { | |||
| e.preventDefault(); | |||
| submitHandler(); | |||
| setIsFilterBoxOpen(false); | |||
| setFilterBoxOpen("__submitted__"); | |||
There was a problem hiding this comment.
Why do we want it to set filterBoxOpen to "__submitted__"? It's slightly unclear how filterBoxOpen gets used when it can equal either filter type or "__submitted__"
PULL REQUEST
Summary
Move control of filter open state from local component to overall component. This allows us to make sure only one component is open at a time and for escape and click handlers to be added to close out filters more easily.
Related Issue
Fixes #2995
Acceptance Criteria
esckey- For example, if the user clicks on condition, the condition box will open. Then, if they clicked on the date filter button, the condition box would automatically close and the filter by date box would open.
- If a filter box is open and the reset button is clicked, then the filter box should close automatically.