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

feat: added multi-select for input parameters. Fixes #12017 #12579

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

ahrakos
Copy link

@ahrakos ahrakos commented Jan 27, 2024

Fixes #12017

Motivation

I made my changes, because in Argo Workflows UI we weren't able to use a dropdown as a multi-select, and instantiate the default value to something that is accepted by a multi-select component.

Modifications

  • I changed the definition of Parameter type, to include another field named multi which is of type boolean. Only when we use the multi-select, we should make sure that the multi value is being set to true, so the UI will treat the select box as a multi-select one, or either a single-select in case of a falsy-value.
  • I changed the UI to consider the multi value, and to plant the relevant callbacks to support the multi-select behavior, as I've seen that the UI library that argo uses already supports multi-select, it's just that no one used it.
Screenshot 2024-01-28 at 0 19 59 Screenshot 2024-01-28 at 0 20 09

On Smaller Screen:


Screenshot 2024-01-29 at 22 02 18
Screenshot 2024-01-29 at 22 02 23

Verification

I saw that the UI doesn't have any kind of tests, so I manually checked that the functionality works in different screens that are using the parameters of type multi-select and single-select.

@ahrakos ahrakos force-pushed the multi-select branch 5 times, most recently from 4faa967 to c5f865c Compare January 27, 2024 23:24
@ahrakos ahrakos marked this pull request as ready for review January 27, 2024 23:46
@agilgur5 agilgur5 self-assigned this Jan 28, 2024
@agilgur5 agilgur5 added area/ui area/spec Changes to the workflow specification. labels Jan 28, 2024
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Could you test the responsiveness of the multi-selection dropdown?

Copy link

@agilgur5 agilgur5 left a comment

Choose a reason for hiding this comment

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

I only took a quick look so far, but there is at least one problem that affects all of the codegen.

pkg/apis/workflow/v1alpha1/workflow_types.go Outdated Show resolved Hide resolved
@ahrakos
Copy link
Author

ahrakos commented Jan 29, 2024

Could you test the responsiveness of the multi-selection dropdown?

Yes, it seems there is no infrastructure for ui tests, is there a preferred way/tools to do that in this project?

@agilgur5
Copy link

agilgur5 commented Jan 29, 2024

Yes, it seems there is no infrastructure for ui tests, is there a preferred way/tools to do that in this project?

There's some unit tests, but no component, functional, or E2E tests for the UI. Although responsiveness is usually tested with visual tests, which are rarer these days too.

Right now, we do a lot of manual testing for visual things, so a screenshot with your browser devtools set to a smaller screen would suffice for this.

I saw that the UI doesn't have any kind of tests, so I manually checked that the functionality works in different screens that are using the parameters of type multi-select and single-select.

i.e. this and attaching screenshots matches our current standards 👍
can do the same thing for responsiveness -- although I'm pretty sure the multi-select has no impact on responsiveness since it's the same size (or about?) as the singular select (and is a dep) 🤷

@ahrakos
Copy link
Author

ahrakos commented Jan 29, 2024

Added two more screenshots of 1024x768 screen size

@ahrakos ahrakos requested a review from agilgur5 January 29, 2024 20:11
@ahrakos ahrakos force-pushed the multi-select branch 2 times, most recently from 5b0ef28 to f3b6eaf Compare January 30, 2024 06:26
@ahrakos
Copy link
Author

ahrakos commented Jan 30, 2024

@agilgur5 it seems that tests are a bit flaky, as at first all of them passed, but now each time I run, a different suite is failing. Can we rerun only the failing suites somehow?

@ahrakos
Copy link
Author

ahrakos commented Feb 6, 2024

@agilgur5
@terrytangyuan

Bumping as no response for more than a week

@agilgur5
Copy link

agilgur5 commented Feb 6, 2024

@agilgur5 it seems that tests are a bit flaky, as at first all of them passed, but now each time I run, a different suite is failing. Can we rerun only the failing suites somehow?

There was a recent flake that was fixed in #12596. It was hit very frequently, so a retry is not necessarily enough for that one. If you rebase, it should be fixed.

I haven't had a chance to review several PRs in-depth yet due to my status, hopefully will get to the simpler ones soon as I recover

@ahrakos
Copy link
Author

ahrakos commented Feb 7, 2024

@agilgur5 it seems that tests are a bit flaky, as at first all of them passed, but now each time I run, a different suite is failing. Can we rerun only the failing suites somehow?

There was a recent flake that was fixed in #12596. It was hit very frequently, so a retry is not necessarily enough for that one. If you rebase, it should be fixed.

I haven't had a chance to review several PRs in-depth yet due to my status, hopefully will get to the simpler ones soon as I recover

Be safe, I had the same issue as well, hope you'll feel better soon.
I rebased and it indeed helped, I'll keep an eye on the PR and wait for you to review :)

@DerrickMartinez

This comment was marked as resolved.

@ahrakos ahrakos force-pushed the multi-select branch 3 times, most recently from 783955f to 8fcec69 Compare March 30, 2024 19:40
@ahrakos
Copy link
Author

ahrakos commented Apr 6, 2024

@agilgur5

Hey again, I hope you are feeling better!
Do you mind taking a look again at the PR?
It's been a while and I think this feature is really important with really small effort :)

I assume @sarabala1979 should look at it as well, so bumping just for a reminder!

@ahrakos ahrakos requested a review from terrytangyuan April 7, 2024 19:55
@ahrakos
Copy link
Author

ahrakos commented Apr 15, 2024

@agilgur5 Any news with that?

@agilgur5 agilgur5 added this to the v3.6.0 milestone May 12, 2024
@agilgur5 agilgur5 changed the title feat: added multi-select for parameters in argo-workflows. Fixes #12017 feat: added multi-select for input parameters. Fixes #12017 Jul 1, 2024
@Joibel Joibel modified the milestones: v3.6.x patches, v3.7.0 Nov 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/spec Changes to the workflow specification. area/ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: multi-select for input parameters
5 participants