feat: Add node table filter #823

ahochsteger · 2020-11-09T23:59:21Z

This PR is a merged and improved version of #582 and #800 and adds the possibility to filter nodes by their column values.

Filtering can done by the following ways:

Search for a sub-string
Select from a list of values
Set min/max number values
Set min/max date values

Screenshots:

Icons have been added to activate the filter options and to indicate a filtered column:

Example of a substring and list selection filter:

Example of min/max number value filter:

Example of min/max date value filter:

lgtm-com · 2020-11-10T00:07:24Z

This pull request introduces 1 alert when merging eb21f5d into a6664ce - view on LGTM.com

new alerts:

1 for Unknown directive

* NOTE: A VSCode setting for correct formatting OOTB would be great

src/modules/NodeCollection.js

robertsLando · 2020-11-10T07:16:46Z

src/components/nodes-table/filter-options.vue

+            <v-text-field type="datetime-local" label="Until" v-model="value.max" clearable></v-text-field>
+          </v-col>
+        </v-row>
+        <v-row v-if="value && (value.type=='string' || value.type=='number' || value.type=='boolean')">


What about use v-switch for booleans?

I'm not sure, if this will work here, since the secure property filter - while being a boolean - can have multiple states:

Do not filter

Filter for true values

Filter for false values

Filter for undefined values

That's why I kept v-select here.
Or do you have a better idea.

@ahochsteger Aren't states: indeterminate true and false? For filtering bools usually checkboxes are used and them can have an indeterminate state (that equals to undefined).

Check it here: https://vuetifyjs.com/en/components/checkboxes/#states

Good idea - will change it to that.

A consequence of changing to 3-state v-switch would be that it would not possible anymore to just show nodes where the secure flag is undefined.
Do you still think, it's better to change it from v-select to v-switch?

Shouldn't all filter checks skip when switch is indeterminate (so the value undefined ) ?

You are right, that this would be inconsistent with other data types.
The only use case where filtering for undefined values might be for debugging Zwave2Mqtt itself but this may not be something a typical user would need.
If you think we don't need it, I will change it.

If you think we don't need it, I will change it.

Ok!

It's done, but I'm not really happy with the resulting layout spacing (caused by v-menu > v-card > v-card-text > v-row > v-col > v-checkbox):

I tried to reduce the nesting (at least for v-checkbox) down to v-menu > v-card > v-checkbox but that resulted in missing padding on the left that I was not able to add using CSS spacing helpers (e.g. pa-.. or ma-...) since they had no effect.
Being a first time Vuetify user I could need some help here.
If you have a better suggestion for the checkbox label, please let me know.

lgtm-com · 2020-11-10T07:18:48Z

This pull request introduces 1 alert when merging 82f1d53 into a6664ce - view on LGTM.com

new alerts:

1 for Unknown directive

src/components/nodes-table/nodes-table.js

src/modules/NodeCollection.js

robertsLando · 2020-11-10T13:26:11Z

@ahochsteger Could you fix lint errors?

chrisns

really happy there's tests ❤️
I do wonder though if we've introduced a lot of alot of rigidity on the table, and how much harder it'll be to say add additional columns

ahochsteger · 2020-11-11T06:31:52Z

@ahochsteger Could you fix lint errors?

Sorry, but I don't find the reason for the following lint errors:

prettier-standard --check
Code style issues found in following files:
docker/store/nodes.json
docker/store/settings.json
src/components/nodes-table/filter-options.vue
src/components/nodes-table/index.vue
src/components/nodes-table/nodes-table.js
src/modules/NodeCollection.js
src/modules/NodeCollection.test.js
store/nodes.json
store/settings.json

It doesn't tell me what's wrong.
Do you have an idea?

chrisns · 2020-11-11T06:53:20Z

@ahochsteger Run without the --check it'll do the fixes for you

robertsLando · 2020-11-11T07:30:13Z

I do wonder though if we've introduced a lot of rigidity on the table, and how much harder it'll be to say add additional columns

Not that harder, just a little bit

ahochsteger · 2020-11-11T07:39:21Z

@ahochsteger Run without the --check it'll do the fixes for you

Thanks, it's done.
That was easy :-)

ahochsteger · 2020-11-11T07:47:27Z

really happy there's tests
I do wonder though if we've introduced a lot of alot of rigidity on the table, and how much harder it'll be to say add additional columns

Kudos for tests go to @mrwho, who did the hard work in #582 on which this PR is based on.
But there seems to be a problem with the coverage/coveralls test that does not see see the new tests.
It reports 0% for NodeCollection.js which has many tests.

Concerning your comment on adding columns:
I tried to be as generic as possible for the table filter so it should be straight forward to extend for more columns.
You are right though, that you have to touch code on a few locations which may be able to encapsulate, but I guess it's out-of-scope for now and could be done, if more demand is here (e.g. if the feature to make it possible to dynamically choose the columns to be shown by the user).

chrisns · 2020-11-11T08:35:01Z

But there seems to be a problem with the coverage/coveralls test that does not see see the new tests.
It reports 0% for NodeCollection.js which has many tests.

yeah, we need to fix the coverage collector, don't worry about that for now.
will pick it up as a separate exercise

robertsLando

LGTM. Thanks @ahochsteger ! :)

ahochsteger · 2020-11-11T11:07:54Z

@robertsLando you are welcome - I'm looking forward to the next release :-).
I guess, #582 is now obsolete and may be closed.

ahochsteger added 3 commits November 10, 2020 00:29

feat: Add node table column filtering

8719584

fix: Add lost nodeTableItems

b7a400c

fix: Remove obsolete filters object

eb21f5d

ahochsteger mentioned this pull request Nov 10, 2020

Add node table column filter #800

Closed

fix: Inconsistent code styling

82f1d53

* NOTE: A VSCode setting for correct formatting OOTB would be great

robertsLando reviewed Nov 10, 2020

View reviewed changes

src/components/nodes-table/nodes-table.js Outdated Show resolved Hide resolved

ahochsteger added 2 commits November 10, 2020 08:31

fix: Remove unknown directive (LGTM alert)

fd4d266

refactor: Use _isUndefined, improve date compare

75fe48d

robertsLando requested a review from chrisns November 10, 2020 08:01

robertsLando reviewed Nov 10, 2020

View reviewed changes

src/components/nodes-table/nodes-table.js Show resolved Hide resolved

robertsLando reviewed Nov 10, 2020

View reviewed changes

src/modules/NodeCollection.js Outdated Show resolved Hide resolved

fix: More robust date comparison in betweenDate()

61e4b94

chrisns approved these changes Nov 10, 2020

View reviewed changes

refactor: Use v-checkbox for boolean column values

efb867d

style: Fix code styling

310deeb

refactor: Move itemsPerPage code to nodes-table

33b63cd

robertsLando approved these changes Nov 11, 2020

View reviewed changes

robertsLando merged commit 33994c0 into OpenZWave:master Nov 11, 2020

ahochsteger mentioned this pull request Dec 18, 2020

feat(ui): add nodes table filter + persistent UI settings zwave-js/zwave-js-ui#90

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add node table filter #823

feat: Add node table filter #823

ahochsteger commented Nov 9, 2020

lgtm-com bot commented Nov 10, 2020

robertsLando Nov 10, 2020

ahochsteger Nov 10, 2020 •

edited

Loading

robertsLando Nov 10, 2020

robertsLando Nov 10, 2020

ahochsteger Nov 10, 2020

ahochsteger Nov 10, 2020

robertsLando Nov 10, 2020

ahochsteger Nov 10, 2020

robertsLando Nov 10, 2020

ahochsteger Nov 11, 2020 •

edited

Loading

lgtm-com bot commented Nov 10, 2020

robertsLando commented Nov 10, 2020

chrisns left a comment

ahochsteger commented Nov 11, 2020

chrisns commented Nov 11, 2020

robertsLando commented Nov 11, 2020

ahochsteger commented Nov 11, 2020

ahochsteger commented Nov 11, 2020

chrisns commented Nov 11, 2020

robertsLando left a comment

ahochsteger commented Nov 11, 2020

feat: Add node table filter #823

feat: Add node table filter #823

Conversation

ahochsteger commented Nov 9, 2020

lgtm-com bot commented Nov 10, 2020

Choose a reason for hiding this comment

ahochsteger Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahochsteger Nov 11, 2020 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Nov 10, 2020

robertsLando commented Nov 10, 2020

chrisns left a comment

Choose a reason for hiding this comment

ahochsteger commented Nov 11, 2020

chrisns commented Nov 11, 2020

robertsLando commented Nov 11, 2020

ahochsteger commented Nov 11, 2020

ahochsteger commented Nov 11, 2020

chrisns commented Nov 11, 2020

robertsLando left a comment

Choose a reason for hiding this comment

ahochsteger commented Nov 11, 2020

ahochsteger Nov 10, 2020 •

edited

Loading

ahochsteger Nov 11, 2020 •

edited

Loading