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

Jump to bottom

feat: Reduce API calls when changing filters. Closes #2231 #2232

Merged
merged 4 commits into from Feb 14, 2020
Merged

feat: Reduce API calls when changing filters. Closes #2231 #2232

merged 4 commits into from Feb 14, 2020

Conversation

whynowy
Copy link
Member

@whynowy whynowy commented Feb 13, 2020 •
edited

Closes #2231.

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 whynowy changed the title feat: Reduce API calls when changing filters feat: Reduce API calls when changing filters. Closes #2231 Feb 13, 2020
@codecov
Copy link

codecov bot commented Feb 13, 2020 •
edited

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2232   +/-   ##
=======================================
  Coverage   11.82%   11.82%           
=======================================
  Files          52       52           
  Lines       26354    26354           
=======================================
  Hits         3116     3116           
  Misses      22837    22837           
  Partials      401      401

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 5d7b4c8...471414f. Read the comment docs.

whynowy
whynowy commented Feb 13, 2020
View reviewed changes
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated
@@ -6,7 +6,13 @@ import {TagsInput} from '../../../shared/components/tags-input/tags-input';

require('./workflow-filters.scss');

export enum FilterType {
Copy link
Member Author

Choose a reason for hiding this comment

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

Archived workflow list page filter will be able to re-use the same component.

ui/src/app/workflows/components/workflows-list/workflows-list.tsx
@@ -115,24 +120,32 @@ export class WorkflowsList extends BasePage<RouteComponentProps<any>, State> {
);
}

private fetchWorkflows(): void {
private fetchWorkflows(namespace: string, selectedPhases: string[], selectedLabels: string[]): void {
Copy link
Member Author

Choose a reason for hiding this comment

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

Change to use passed in variables because it seems like setState is async and using this.state.xxx right after setState does not return latest value.

@whynowy whynowy requested a review from alexec February 13, 2020 19:06
alexec
alexec reviewed Feb 13, 2020
View reviewed changes
ui/src/app/workflows/components/workflow-filters/workflow-filters.tsx Outdated
interface WorkflowFilterProps {
type: FilterType;
Copy link
Contributor

Choose a reason for hiding this comment

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

this creates tight coupling between the components - I think you need to decouple them

Copy link
Member Author

@whynowy whynowy Feb 14, 2020 •
edited

Choose a reason for hiding this comment

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

updated.

alexec
alexec requested changes Feb 13, 2020
View reviewed changes
Copy link
Contributor

@alexec alexec left a comment

Choose a reason for hiding this comment

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

See comments.

@whynowy whynowy requested a review from alexec February 13, 2020 20:26
simster7
simster7 reviewed Feb 13, 2020
View reviewed changes
ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated
.then(info => {
if (info.managedNamespace && info.managedNamespace !== this.state.namespace) {
this.setState({namespace: info.managedNamespace});
let l;
Copy link
Member

Choose a reason for hiding this comment

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

Can we have full variable names?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

ui/src/app/workflows/components/workflows-list/workflows-list.tsx Outdated
if (info.managedNamespace && info.managedNamespace !== this.state.namespace) {
this.setState({namespace: info.managedNamespace});
let l;
let ns = namespace;
Copy link
Member

Choose a reason for hiding this comment

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

Same here

Copy link
Member Author

Choose a reason for hiding this comment

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

updated.

@whynowy whynowy merged commit af94352 into argoproj:master Feb 14, 2020
@whynowy whynowy deleted the wflistenhance branch February 14, 2020 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simster7 simster7 simster7 approved these changes

@alexec alexec alexec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce API calls when changing filters on wf list page
3 participants
@whynowy @simster7 @alexec