feat(ui): Add label filter to workflow list page. Fixes #802 #2196

whynowy · 2020-02-07T22:19:26Z

Fixes: #802

Checklist:

Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this is a chore.
The title of the PR is (a) conventional, (b) states what changed, and (c) suffixes the related issues number. E.g. "fix(controller): Updates such and such. Fixes #1234".
I have written unit and/or e2e tests for my change. PRs without these are unlike to be merged.
Optional. I've added My organization is added to the README.
I've signed the CLA and required builds are green.

whynowy · 2020-02-07T22:23:30Z

ui/src/app/shared/components/input-filter.tsx

+    error?: Error;
+}
+
+export class InputFilter extends React.Component<InputProps, InputState> {


This is abstracted from original NamespaceFilter to make it a generic input box with local cache.

whynowy · 2020-02-07T22:24:19Z

ui/src/app/shared/components/namespace-filter.tsx

-                    <i className='fa fa-times-circle' />
-                </a>
-            </>
+            <InputFilter


Reference to abstracted InputFilter

whynowy · 2020-02-07T22:25:30Z

ui/src/app/shared/components/namespace-filter.tsx

-            </>
+            <InputFilter
+                value={this.namespace}
+                placeholder='Namespace'


Use placeholder to replace Namespace text.

whynowy · 2020-02-07T22:26:45Z

ui/src/app/shared/components/tags-input/tags-input-field.tsx

@@ -0,0 +1,16 @@
+import * as React from 'react';


tags-input is introduced from argo-cd.

whynowy · 2020-02-07T22:28:33Z

ui/src/app/shared/components/checkbox-filter/checkbox-filter.tsx

@@ -0,0 +1,53 @@
+import * as React from 'react';


checkbox-filter introduces a list of checkboxes with counts displayed for for each item.

whynowy · 2020-02-07T22:30:55Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

        return (
            <Consumer>
                {ctx => (
                    <Page
                        title='Workflows'
                        toolbar={{
-                            filter,


Phase filter is now part of workflow-filters.

whynowy · 2020-02-07T22:31:24Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

@@ -104,9 +96,20 @@ export class WorkflowsList extends BasePage<RouteComponentProps<any>, State> {
                                    }
                                ]
                            },
-                            tools: [<NamespaceFilter key='namespace-filter' value={this.namespace} onChange={namespace => (this.namespace = namespace)} />]


Namespace filter is now also part of workflow-filters

codecov · 2020-02-07T22:35:51Z

Codecov Report

Merging #2196 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #2196   +/-   ##
======================================
  Coverage    11.7%   11.7%           
======================================
  Files          52      52           
  Lines       26313   26313           
======================================
  Hits         3081    3081           
  Misses      22837   22837           
  Partials      395     395

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2d3416b...0381986. Read the comment docs.

ui/src/app/shared/components/checkbox-filter/checkbox-filter.tsx

alexec

looks pretty good so for - some minor comments - it might be good to do a side-by-side review on Monday - maybe include @simster7 ?

ui/src/app/shared/components/input-filter.tsx

alexec · 2020-02-07T23:59:59Z

ui/src/app/shared/components/namespace-filter.tsx

@@ -30,17 +28,7 @@ export class NamespaceFilter extends React.Component<Props, State> {
    }

    private set namespace(namespace: string) {
-        this.setState(state => {


with this code removed - how do entries get saved into local storage?

It's moved to generic component input-filter.

If all these getters and setters do is modify and read from state I would recommend not using them and simply modifying and reading state directly. Way more explicit and React-y

alexec · 2020-02-08T00:00:57Z

ui/src/app/shared/components/input-filter.tsx

+
+interface InputState {
+    input: string;
+    localInputs: string[];


I'm not sure about this name - would something like previousValue be good?

Renamed to localCache

ui/src/app/shared/components/input-filter.tsx

alexec · 2020-02-08T00:01:42Z

ui/src/app/shared/components/input-filter.tsx

+interface InputProps {
+    value: string;
+    placeholder: string;
+    type: string;


what is "type"? is in select or checkbox?

It is used to generate the local storage key. I renamed it to name.

alexec · 2020-02-08T00:02:57Z

ui/src/app/shared/components/tags-input/tags-input.tsx

+
+require('./tags-input.scss');
+
+export class TagsInput extends React.Component<TagsInputProps, {tags: string[]; input: string; focused: boolean}> {


@alexmt should we be pushing this into the shared Argo UI code? does that still exist?

ui/src/app/shared/components/tags-input/tags-input.tsx

alexec · 2020-02-08T00:03:58Z

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

+    error?: Error;
+}
+
+export class WorkflowFilters extends React.Component<WorkflowFilterProps, WorkflowFilterState> {


I like how we have one big component that comprises of smaller ones

ui/src/app/shared/components/checkbox-filter/checkbox-filter.scss

simster7

Good work for your first React PR! Jotted down some initial comments, but this should have a more in-depth review soon. A couple of things:

The design is not responsive:

It might be just my build, but it seems like this is very broken and slow. See: https://cl.ly/4d454b27345a. This will need some reworking and actual testing before it's ready.
This needs a make lint

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

simster7 · 2020-02-08T20:50:02Z

ui/src/app/workflows/components/workflows-list/workflows-list.tsx

+                                        namespace={this.namespace}
+                                        phases={this.phases}
+                                        labels={this.labels}
+                                        onChange={(namespace, phases, labels) => this.handleChanges(namespace, phases, labels)}


Could simply pass in the function here

Suggested change

onChange={(namespace, phases, labels) => this.handleChanges(namespace, phases, labels)}

onChange={this.handleChanges}

Somehow it doesn't work.....

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

simster7 · 2020-02-08T21:02:05Z

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

+            namespace: props.namespace,
+            phases: props.phases,
+            labels: props.labels


Not too sure about this pattern... there should only be one source of truth about what the current namespace, phases, and labels are. Right now we are storing it in three different places: the URL, state in workflows-list, and this state. It should all be consolidated in one. Why store state here if you pass in an onChange that changes the state of the parent component... forcing a re-render here anyway?

Realized the same thing, removed the state and used the props.

ui/src/app/shared/components/tags-input/tags-input-field.tsx

simster7 · 2020-02-08T21:19:59Z

ui/src/app/shared/components/tags-input/tags-input.tsx

+                        }
+                    }}
+                    renderInput={props => (
+                        <input


This whole <input> field doesn't seem like it is the correct way to do this. Can you check how we take input elsewhere in the codebase?

ui/src/app/shared/services/workflows-service.ts

simster7 · 2020-02-08T21:22:20Z

ui/src/app/shared/services/workflows-service.ts

+        }
+        if (filter.labels && filter.labels.length > 0) {
+            if (labelSelector.length > 0) {
+                labelSelector += ',';
+            }
+            labelSelector += filter.labels.join(',');
+        }
+        if (labelSelector.length > 0) {
+            queryParams.push(`listOptions.labelSelector=${labelSelector}`);


The labelSelector logic should be extracted as a utility function

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

whynowy · 2020-02-09T23:02:33Z

Good work for your first React PR! Jotted down some initial comments, but this should have a more in-depth review soon. A couple of things:

The design is not responsive:

It might be just my build, but it seems like this is very broken and slow. See: https://cl.ly/4d454b27345a. This will need some reworking and actual testing before it's ready.

This needs a make lint

I noticed the slow responsive issue, it seems like the API call /api/v1/workflow-events/ is stuck, there must be some issue on the server side. I'll take a further look.

simster7

Excellent work! This is a great PR. Conditional approval: please fix the un-responsive design. I.e. the site doesn't resize well with different screen sizes. See: https://cl.ly/bd0c78ca10df

simster7 · 2020-02-11T02:42:20Z

ui/src/app/shared/components/input-filter.tsx

+import {Autocomplete} from '../../../../node_modules/argo-ui';
+
+interface InputProps {
+    value: string;


Nit pick: maybe call this initialValue?

Thanks!

I feel like value would be better, since it's still a <input type="text">, using value to be consistent with it.

simster7 · 2020-02-11T02:47:12Z

ui/src/app/shared/components/input-filter.tsx

+        };
+    }
+
+    private set value(value: string) {


This setter should be converted to a function and renamed something like setValueAndCache

simster7 · 2020-02-11T02:52:53Z

ui/src/app/shared/components/tags-input/tags-input.tsx

+require('./tags-input.scss');
+
+export class TagsInput extends React.Component<TagsInputProps, {tags: string[]; input: string; focused: boolean}> {
+    private inputEl: HTMLInputElement;


Nit pick:

Suggested change

private inputEl: HTMLInputElement;

private inputElement: HTMLInputElement;

simster7 · 2020-02-11T02:56:09Z

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

+                        <NamespaceFilter
+                            value={this.props.namespace}
+                            onChange={ns => {
+                                this.props.onChange(ns, this.props.selectedPhases, this.props.selectedLabels);


Much better!

simster7 · 2020-02-11T02:57:22Z

ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx

+                    const value = wf.metadata.labels[label];
+                    const suggestedLabel = `${label}=${value}`;
+                    if (!suggestions.some(v => v === suggestedLabel)) {
+                        suggestions.push(`${label}=${value}`);


Maybe don't include labels that denote phase because the check boxes already provide that functionality? Up to you

whynowy · 2020-02-11T06:18:06Z

Excellent work! This is a great PR. Conditional approval: please fix the un-responsive design. I.e. the site doesn't resize well with different screen sizes. See: https://cl.ly/bd0c78ca10df

Thanks! Fixed.

feat(ui): Add label filter

1c4457c

whynowy changed the title ~~feat(ui): Add label filter to workflow list page. Fixes#802~~ feat(ui): Add label filter to workflow list page. Fixes #802 Feb 7, 2020

whynowy requested review from alexec and simster7 February 7, 2020 22:21

whynowy commented Feb 7, 2020

View reviewed changes

fix yarn lint error

a15004e

Move search box on top of filters

a996861

alexec reviewed Feb 7, 2020

View reviewed changes

ui/src/app/shared/components/checkbox-filter/checkbox-filter.tsx Outdated Show resolved Hide resolved

alexec reviewed Feb 8, 2020

View reviewed changes

alexmt reviewed Feb 8, 2020

View reviewed changes

ui/src/app/shared/components/checkbox-filter/checkbox-filter.scss Outdated Show resolved Hide resolved

address feedbacks

3758150

simster7 suggested changes Feb 8, 2020

View reviewed changes

enhancement

23a1aab

whynowy added 4 commits February 9, 2020 15:03

fix lint issue

3280d1a

Merge branch 'master' into labelselector

a30d814

bugfixx

2d3416b

Merge remote-tracking branch 'upstream/master' into labelselector

f2761fe

whynowy mentioned this pull request Feb 11, 2020

Can not unselect the last phase checkbox in phase selector of argo-server workflow list page #2169

Closed

4 tasks

simster7 approved these changes Feb 11, 2020

View reviewed changes

whynowy added 2 commits February 10, 2020 22:10

changes

2af7c12

fix lint error

0381986

whynowy merged commit e30b77f into argoproj:master Feb 11, 2020

whynowy deleted the labelselector branch February 11, 2020 06:36

simster7 mentioned this pull request Feb 12, 2020

fix: UI Responsive design on filter box #2221

Merged


		require('./tags-input.scss');

		export class TagsInput extends React.Component<TagsInputProps, {tags: string[]; input: string; focused: boolean}> {

	onChange={(namespace, phases, labels) => this.handleChanges(namespace, phases, labels)}
	onChange={this.handleChanges}

	private inputEl: HTMLInputElement;
	private inputElement: HTMLInputElement;

feat(ui): Add label filter to workflow list page. Fixes #802 #2196

feat(ui): Add label filter to workflow list page. Fixes #802 #2196

Conversation

whynowy commented Feb 7, 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

codecov bot commented Feb 7, 2020 • edited Loading

Codecov Report

alexec left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simster7 left a 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

whynowy commented Feb 9, 2020

simster7 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whynowy commented Feb 11, 2020

whynowy commented Feb 7, 2020 •

edited

Loading

codecov bot commented Feb 7, 2020 •

edited

Loading