-
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(ui): add history to WorkflowTemplate & ClusterWorkflowTemplate details #13452
Conversation
9c7ffce
to
e00afbf
Compare
Signed-off-by: panicboat <panicboat@gmail.com>
e00afbf
to
301db1d
Compare
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.
Please see #13283 (comment)
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 looks much better and simpler.
I left one correction of a comment below. The other comments are more for future consideration.
Otherwise LGTM, nice work 🙂
ui/src/app/workflows/components/workflow-details-list/workflow-details-list.tsx
Outdated
Show resolved
Hide resolved
ui/src/app/workflows/components/workflow-details-list/workflow-details-list.tsx
Show resolved
Hide resolved
setWorkflows(workflowList.items); | ||
setColumns(workflowsInfo.columns); | ||
})(); | ||
}, []); |
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.
we may want to consider adding name
as a hook dep in the array here, but this is identical to cron-workflow-details.tsx
right now, so this is fine.
ui/src/app/cluster-workflow-templates/cluster-workflow-template-details.tsx
Show resolved
Hide resolved
Signed-off-by: Anton Gilgur <4970083+agilgur5@users.noreply.github.com>
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.
Made the one remaining correction myself, so LGTM now
Fixes #13282
I didn't have the authority to reopen the #13283, so I am creating a new one.
Motivation
As indicated in the Issue, it is inconvenient that the history is not visible in the WorkflowTemplate details.
CronWorkflow can see it, so I would like to see consistency.
Modifications
WorkflowTemplate
No history
ClusterWorkflowTemplate
No history
Verification