Skip to content
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

Remove broken isAnyOf filter in lists/views #1565

Merged
merged 4 commits into from
Sep 29, 2023

Conversation

niklasva82
Copy link
Member

Description

This PR removes the broken isAnyOf filter in lists, and customizes the filter type for the local_bool type. In the future we should probably have the filterOperators based on the type of column, but a lot of these seem to need custom functions because e.g. dates are not stored as dates in the cells.

Screenshots

image
image

Notes to reviewer

I tried to solve the underlying issue, which is that GridFilterInputMultipleValues sometimes receives a single string rather than an array of strings, which crashes the page. Perhaps this could be solved in the future if we think the "any of" filter is important.

Related issues

Resolves #1544

@richardolsson
Copy link
Member

I'm guessing this is ready for review?

@niklasva82
Copy link
Member Author

niklasva82 commented Sep 29, 2023 via email

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In your PR intro you put:

In the future we should probably have the filterOperators based on the type of column, but a lot of these seem to need custom functions because e.g. dates are not stored as dates in the cells.

I agree with this, and it's actually already being done for some column types, e.g. the local_query type:

Filtering a local_query column

It can also be achieved quite easily for other column types, e.g. survey_submitted which should be interpreted as a date:

diff --git a/src/features/views/components/ViewDataTable/columnTypes/SurveySubmittedColumnType.tsx b/src/features/views/components/ViewDataTable/columnTypes/SurveySubmittedColumnType.tsx
index c973b302..519b3cee 100644
--- a/src/features/views/components/ViewDataTable/columnTypes/SurveySubmittedColumnType.tsx
+++ b/src/features/views/components/ViewDataTable/columnTypes/SurveySubmittedColumnType.tsx
@@ -29,6 +29,8 @@ export default class SurveySubmittedColumnType
         return <Cell cell={params.value} />;
       },
       width: 250,
+      type: 'dateTime',
+      valueGetter: (params) => new Date(params.value.submitted),
     };
   }
   getSearchableStrings(): string[] {

But this PR is actually a step in the opposite direction, because it overrides all filter operators (except local_bool), regardless of column data type and other configuration options in the colDef, to always use string operators.

In my opinion, this is not a good solution, because it not only removes functionality that currently exists (e.g. boolean filtering of Smart Search columns, screenshot above), but it also makes future changes more difficult by adding this override that whoever comes back to add more flexible filters will have a very hard time finding.

I think another approach to this is needed, and I'd really wish that we could solve the underlying issue. Maybe if you could elaborate on it, we could work it out together?

if (COLUMN_TYPE.LOCAL_BOOL === col.type) {
return getGridBooleanOperators();
} else {
return getGridStringOperators().filter((op) => op.value !== 'isAnyOf');
Copy link
Member

@richardolsson richardolsson Sep 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means all columns except local_bool get handled as strings, regardless of how they've been marked up in the column definition.

Copy link
Member

@richardolsson richardolsson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this came out good and the diff looks nice and clean to me. I suggest you use the GitHub feature to "squash and merge" as a single commit. 😊

@niklasva82 niklasva82 merged commit 8567a59 into main Sep 29, 2023
@niklasva82 niklasva82 deleted the issue-1544/remove-isanyof-filter branch September 29, 2023 21:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement "is any of" filter in lists
2 participants