-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
feat: Reduce API calls when changing filters. Closes #2231 #2232
Conversation
Codecov Report
@@ 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.
|
@@ -6,7 +6,13 @@ import {TagsInput} from '../../../shared/components/tags-input/tags-input'; | |||
|
|||
require('./workflow-filters.scss'); | |||
|
|||
export enum FilterType { |
There was a problem hiding this comment.
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.
@@ -115,24 +120,32 @@ export class WorkflowsList extends BasePage<RouteComponentProps<any>, State> { | |||
); | |||
} | |||
|
|||
private fetchWorkflows(): void { | |||
private fetchWorkflows(namespace: string, selectedPhases: string[], selectedLabels: string[]): void { |
There was a problem hiding this comment.
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.
interface WorkflowFilterProps { | ||
type: FilterType; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
ui/src/app/workflows/components/workflows-list/workflows-list.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/workflows/components/workflows-list/workflows-list.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments.
.then(info => { | ||
if (info.managedNamespace && info.managedNamespace !== this.state.namespace) { | ||
this.setState({namespace: info.managedNamespace}); | ||
let l; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
if (info.managedNamespace && info.managedNamespace !== this.state.namespace) { | ||
this.setState({namespace: info.managedNamespace}); | ||
let l; | ||
let ns = namespace; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
Closes #2231.
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.