Add filtering by undefined and fix searching by name #2714

joshri · 2022-09-06T21:40:30Z

Closes #2711
Closes #2712

Fixed a bug where filter categories would be disabled if their first value was undefined
Fixed display of undefined values in data across the app (in all tables, filter dialogs, and chips). Empty data is now represented by -
Fixed a bug in the filterText function that didn't allow searching by name with the new object structure (name can be a function instead of a string). The function now finds the row's value as other filtering functions do - with field.value(row) or row[field.value], which supports both strings and functions.
Fixed a styling bug that adjusted content when chips appeared. ChipGroup is now set at 40px
Changes in tests allow for us to change what the filter separator is without breaking everything. I also added a test in ChipGroup to test if changing chips that end with the filter separator add a - to properly display undefined.

Quick vid showing filtering and searching on the notifications page:

FilterUndefined.mov

ozamosi · 2022-09-08T12:11:49Z

ui/components/DataTable.tsx

-          return f.sortValue(row) === value;
-        }
-      });
+    fields.forEach((field) => {


I think this needs to be simplified further. Let's say that both name and namespace (in that order) is text searchable. If you search for 1 text filter that matches the name, then I think the namespace would set matches back to false. So I think the matches filter needs to be more carefully passed from "layer to layer", instead of just building up one value as we loop.

The text filters seem like they should be ANDed based on the tests, so I'm going to assume that's correct.

To make this work properly, I think we need to:

First (inner-most) OR each field together - any field that matches a single text filter means that the whole row matches that filter.

Then, AND the filters together.

And only then do we decide if the row as a whole matches the filters as a whole.

We're currently merging the filters before we're merging the columns, which I think doesn't work properly with multiple textSearchable columns - if again name and namespace are searchable, you wouldn't get a hit if one filter matches name and one filter matches namespace, only if all filters matches either name or namespace.

So I would change this whole loop to:

return rows.filter((row) => { return textFilters.every((filter) => { return fields.some((field) => { if (!field.textSearchable) return false; let value; if (field.sortValue) { value = field.sortValue(row); } else if (typeof field.value === "function") { value = field.value(row); } else { value = row[field.value]; } return value.includes(filter); }); }); });

Perhaps we could even turn that value extraction into some helper function.

I like chaining together every and some because they help separate "checking the condition" from "combining the outputs" so they take care of the matches malarkey for us - we only return a boolean, and the right thing comes out, and they even handles short-circuiting for us.

I don't think it was ever intended to text search any column other than name - I took a look through and no tables have more than one column searchable. I'm down to add this because why not - but to me it'd be weird to have something be filterable AND text searchable.

Text search let you do substring matches, the filter UI doesn't - so you can't filter for "all namespaces with denim in the name" without going through them all.

Should I add textSearchable on all Namespace and Cluster columns? Any other columns that might be useful?

ozamosi · 2022-09-08T12:15:02Z

ui/components/ChipGroup.tsx

-            <Chip label={chip} onDelete={() => onChipRemove([chip])} />
+            <Chip
+              label={
+                chip.slice(-filterSeparator.length) === `${filterSeparator}`


Either do something like chip.indexOf(filterSeparator) == chip.length - filterSeparator.length (I probably have an off-by-one in there, I haven't tried it) so it 100% only manipulates empty chip values, or just drop this.

@joshri why are you using a template literal

`${filterSeparator}`

here instead of just filterSeparator? Was the comparison not working?

opudrovs · 2022-09-08T20:33:16Z

How can the user determine which fields are searchable? I see the textSearchable prop, but only as a developer, is there a visual indication for the fields you can search? The first thing I tried searching was by channel and type, it didn't work. Then I read that search is enabled only for names.

opudrovs · 2022-09-08T20:51:32Z

If using the Chip component imported from Material-UI causes such problems, can we just create our own Chip component which has two props, label and value (or maybe not label to avoid confusion with the original Chip component), and combines them as the label of the original Chip component it renders with a filterSeparator const, like return <OriginalChipComponent label={${OurChipComponentLabelProp}${filterSeparator}${OurChipComponentValueProp}}>? So, we can always know that we are not messing up a real value.

joshri · 2022-09-12T15:15:22Z

Created some follow-up issues - for now I've removed the hack in ChipGroup and left the filterByText function as is

…s in the frontend, and allow for undefined values as - in filters

opudrovs · 2022-09-12T16:53:57Z

The ChipGroup tests are failing, including locally for me.

opudrovs · 2022-09-12T18:11:47Z

ui/components/DataTable.tsx

+      if (field.sortValue) {
+        value = field.sortValue(row);
+      } else {
+        typeof field.value === "function"


Can we re-write this ternary as:

value = typeof field.value === "function" ? field.value(row) : row[field.value];

It's a standard form of an assignment with ternary instead of having multiple assignments.

opudrovs · 2022-09-12T18:13:50Z

ui/components/DataTable.tsx

-      for (const filterString of textFilters) {
-        if (_.includes(value, filterString)) {
+      for (let i = 0; i < textFilters.length; i++) {
+        if (value.includes(textFilters[i])) {


This can be refactored as

matches = value.includes(textFilters[i]); if (!matches) { break; }

It would be easier to read.

opudrovs · 2022-09-12T18:23:43Z

Is Not Ready text in the status column supposed to wrap like this? Or do we need to make the status column wider?

opudrovs · 2022-09-12T18:39:05Z

Is it expected behavior that an empty filter dialog is displayed for the events table? I don't think it's specific to this PR.

I remember fixing sorting in the events table but don't remember working with filters in it. Is the events table supposed to have filters or should we add hiding the filter dialog button for it?

opudrovs

Tested it, have not found a way to break anything! 🎉 Except for the failing test, LGTM.

joshri · 2022-09-12T19:55:55Z

@opudrovs

"Not Ready" wrapping - I don't believe it's intentional that it's wrapping, but I also don't see any limits on the width of the status column

Events Table empty filters - we aren't providing a filter state to the table so it makes sense that it's empty. We should probably disable search and filtering on this table with a boolean prop on DataTable

joshri · 2022-09-12T20:14:22Z

We decided to make the filter dialog optional on the data table since the events table has never had any filtering functionality

joshri added the area/ui Issues that require front-end work label Sep 6, 2022

joshri requested review from ozamosi and opudrovs September 6, 2022 21:40

ozamosi reviewed Sep 8, 2022

View reviewed changes

This was referenced Sep 12, 2022

Multi-column text search #2724

Closed

Add some sort of design in how chip text is displayed #2725

Closed

joshri added 19 commits September 12, 2022 11:18

empty settings page and route

c4c445f

wow it was annoying to make the settings page

e24f6d4

did not end up needing to change breadcrumbs

921dcc6

list providers

aa1dfcf

debug adding notifications in the backend, debug provider object clas…

0a928c6

…s in the frontend, and allow for undefined values as - in filters

filter by name search was broken and tests were not behaving as expected

59cdc1b

go mod tidy

8a48d26

filterByText formatting

7bf8ae1

filter dialog formatting

2fc7531

filter bugs and added filterSeparator to tests

e14e615

removed an oopsie yaml file

1d65ffe

one spot where filterSeparator was missing

e0cd849

bad fix for undefined chip label

1505f2a

SNAPS

eba71c2

better test for displaying dash in chip, chip group height

4691a0a

test changes

8ecf8f6

snapshot

59a9f60

remove override for chip text

abefa63

test updates

5a4d7bb

joshri force-pushed the filter-undefined branch from d1a2c86 to 5a4d7bb Compare September 12, 2022 15:18

ozamosi approved these changes Sep 12, 2022

View reviewed changes

opudrovs reviewed Sep 12, 2022

View reviewed changes

opudrovs approved these changes Sep 12, 2022

View reviewed changes

remove screen.debug and make filterByText better

9828e92

make search and filter optional on data table

771e0e3

joshri merged commit 7b7ceff into main Sep 12, 2022

joshri deleted the filter-undefined branch September 12, 2022 20:40

This was referenced Sep 14, 2022

Updates for 0.9.5-rc.1 #2742

Closed

Updates for 0.9.5-rc.1 #2743

Merged

Updates for 0.9.5 #2747

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add filtering by undefined and fix searching by name #2714

Add filtering by undefined and fix searching by name #2714

joshri commented Sep 6, 2022 •

edited

Loading

ozamosi Sep 8, 2022

joshri Sep 8, 2022

ozamosi Sep 8, 2022

joshri Sep 8, 2022

ozamosi Sep 8, 2022

opudrovs Sep 8, 2022

opudrovs commented Sep 8, 2022 •

edited

Loading

opudrovs commented Sep 8, 2022 •

edited

Loading

joshri commented Sep 12, 2022

opudrovs commented Sep 12, 2022 •

edited

Loading

opudrovs Sep 12, 2022 •

edited

Loading

opudrovs Sep 12, 2022

opudrovs commented Sep 12, 2022 •

edited

Loading

opudrovs commented Sep 12, 2022

opudrovs left a comment •

edited

Loading

joshri commented Sep 12, 2022

joshri commented Sep 12, 2022

Add filtering by undefined and fix searching by name #2714

Add filtering by undefined and fix searching by name #2714

Conversation

joshri commented Sep 6, 2022 • edited Loading

ozamosi Sep 8, 2022

Choose a reason for hiding this comment

joshri Sep 8, 2022

Choose a reason for hiding this comment

ozamosi Sep 8, 2022

Choose a reason for hiding this comment

joshri Sep 8, 2022

Choose a reason for hiding this comment

ozamosi Sep 8, 2022

Choose a reason for hiding this comment

opudrovs Sep 8, 2022

Choose a reason for hiding this comment

opudrovs commented Sep 8, 2022 • edited Loading

opudrovs commented Sep 8, 2022 • edited Loading

joshri commented Sep 12, 2022

opudrovs commented Sep 12, 2022 • edited Loading

opudrovs Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

opudrovs Sep 12, 2022

Choose a reason for hiding this comment

opudrovs commented Sep 12, 2022 • edited Loading

opudrovs commented Sep 12, 2022

opudrovs left a comment • edited Loading

Choose a reason for hiding this comment

joshri commented Sep 12, 2022

joshri commented Sep 12, 2022

joshri commented Sep 6, 2022 •

edited

Loading

opudrovs commented Sep 8, 2022 •

edited

Loading

opudrovs commented Sep 8, 2022 •

edited

Loading

opudrovs commented Sep 12, 2022 •

edited

Loading

opudrovs Sep 12, 2022 •

edited

Loading

opudrovs commented Sep 12, 2022 •

edited

Loading

opudrovs left a comment •

edited

Loading