- Notifications
You must be signed in to change notification settings - Fork923
feat: allow entering non-default values in multi-select#15935
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| @@ -101,6 +101,39 @@ func TestMultiSelect(t *testing.T) { | |||
| }() | |||
| require.Equal(t, items, <-msgChan) | |||
| }) | |||
| t.Run("MultiSelectWithCustomInput", func(t *testing.T) { | |||
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.
couldn't add tests to check the custom input flow, we had this check here and not sure if its safe to remove and add interactive tests, please do let me know if there's a better way to incorporate those tests
Line 310 in63572d9
| ifflag.Lookup("test.v")!=nil { |
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.
I think historically we've manually tested this usingprompt-example. I agree that it's not ideal :-(
Hi@johnstcn , could you please have a look at this PR? I do have a question regarding duplicates, if users enters the same custom value twice should we treat it as a single option ? right now it will create 2 duplicate options, what do you think will be the better way to handle that scenario? |
I think it would make more sense to either de-duplicate or validate that the custom option isn't already present. |
cli/cliui/select.go Outdated
| message string | ||
| canceled bool | ||
| selected bool | ||
| isInputMode bool // New field to track if we're adding a custom option |
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.
suggestion:isCustomInputMode for consistency
| m.cursor =len(options) - 1 | ||
| m.cursor =maxIndex |
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.
I think this logic needs to handle whenEnableCustomInput is false. Currently it lets you go over the last valid option.
cli/prompts.go Outdated
| }, | ||
| Defaults: []string{"Code"}, | ||
| Defaults: []string{"Code"}, | ||
| EnableCustomInput: true, |
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 expose this as a flag so we can easily test both with and without custom input?
I've addressed the comments and handled duplicates as shown in the recording
Screen.Recording.2024-12-20.at.11.39.55.AM.mov |
cli/cliui/select.go Outdated
| // Check for duplicates | ||
| for i, opt := range m.options { | ||
| if strings.EqualFold(opt.option, m.customInput) { |
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.
I'm not 100% sure about the case-insensitive matching. On the surface, it seems innocuous enough but I'm not sure if there's going to be a case where someone is going to want to differentiateA froma. Let's keep matching case-sensitive for now.
We can add an option for case-insensitive matching later if someone requests it.
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 the PR!
638247c intocoder:mainUh oh!
There was an error while loading.Please reload this page.
Closes#15488
cliui.Multiselect currently does not allow users to input custom options if zero choices are provided.
Proposed Solution
cliui.MultiSelect should always allow users to specify another option.
For example, when running coder exp prompt-example multi-select:
PS: I have modified the text a bit from the original issue
Attaching a recording for the same
Screen.Recording.2024-12-19.at.4.25.12.PM.mov