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

fix: cron workflow initial filter value. Fixes #11685 #11686

Merged
merged 1 commit into from Aug 27, 2023

Conversation

zel0rd
Copy link
Contributor

@zel0rd zel0rd commented Aug 27, 2023

Fixes #11685

Modifications

Encountering some issues while using the Cron workflow feature.

Modifications

  1. When I enter the cron workflow, the lists are displayed, but all the filters are removed and set to default values. This can confuse users compared to having all the phases filters selected from the beginning. Therefore, the state of the cron workflow should start with all selections made.스크린샷 2023-08-27 오전 2 14 12

  2. The second issue is related to modifying the cron in the cron workflow. The style of the Concurrency Policy is broken. The position and height of the Select box and Select arrow need to be adjusted.스크린샷 2023-08-27 오전 2 06 07

Signed-off-by: zel0rd <zelord.kwoun@gmail.com>
@zel0rd zel0rd marked this pull request as ready for review August 27, 2023 08:51
@terrytangyuan terrytangyuan merged commit 9cb3783 into argoproj:master Aug 27, 2023
23 checks passed
@@ -13,6 +13,7 @@ interface Props {
export class CheckboxFilter extends React.Component<Props> {
constructor(props: any) {
super(props);
this.props.items.forEach(item => this.props.selected.push(item.name));
Copy link
Member

@agilgur5 agilgur5 Aug 27, 2023

Choose a reason for hiding this comment

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

mmm this is a bit of an anti-pattern, especially for a reusable component like this one (it's used in more than one place). The anti-pattern is more visible in a functional component; it would be modifying an argument.

It would be better to just set the initial selected props from the parent component. That ends up going two parents up with the initial state here

Copy link
Member

Choose a reason for hiding this comment

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

Fixed in #11739

dpadhiar pushed a commit to dpadhiar/argo-workflows that referenced this pull request May 9, 2024
…oj#11686)

Signed-off-by: Dillen Padhiar <dillen_padhiar@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ui: Cron workflow initial filter value
3 participants