CDE-62 Connect filter dialog and functionalities #30
Conversation
|
I instructed @Aiga115 to start from my branch because I had fixed a couple of problems there that she reported to me in private while was trying to resolve this issue. With this context, I changed the target branch from develop to feature/CDE-38 as it facilitates the review of @Aiga115's work for CDE-62 |
There was a problem hiding this comment.
Looks good but the reset filter is not connected:
I also think that given the filters structure we should always have at least one filter selected. Let me explain why I think that with a question:
If the filters state is the following what should we show? cc @syamace-metacell @ddelpiano
| @@ -91,6 +91,12 @@ interface MappingProps { | |||
| defaultCollection: string; | |||
| } | |||
|
|
|||
| interface CheckedState { | |||
There was a problem hiding this comment.
Suggestion: Unless there's a reason to, we should keep just one of the CheckedState interfaces. I see that it is defined both in the Mapping Tab and in the Mapping Search components
| const isAnyTrue = Object.values(checked).some(value => value === true); | ||
|
|
||
| if (isAnyTrue) { | ||
| return checked[entityType]; |
There was a problem hiding this comment.
Note (non-blocking): This assumes that all filters will be on the row type. Ideally I would prefer a more generic approach but I won't mark it as a blocker since the code correctly addresses the current scope.
| } | ||
|
|
||
| export default function MappingSearch({onChange}: MappingSearchProps) { | ||
|
|
||
| const [searchString, setSearchString] = useState(''); | ||
| const [checked, setChecked] = React.useState({ |
There was a problem hiding this comment.
suggestion (non-blocking): Rename 'checked' to something more informative like for example filtersState
lib/models.ts
Outdated
| @@ -79,6 +79,12 @@ export interface SelectableCollection { | |||
| selected: boolean; | |||
| } | |||
|
|
|||
| export interface CheckedState { | |||
There was a problem hiding this comment.
Suggestion (non-blocking): Rename CheckedState to FiltersState
Issue #CDE-62
Problem: Connect filter dialog and functionalities
Solution:
Add feature to filter data according to selected checkbox values
Recording.2024-03-11.123148.mp4