Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Added missing AskOpts #216

Merged
merged 21 commits into from
Jun 3, 2019
Merged

Added missing AskOpts #216

merged 21 commits into from
Jun 3, 2019

Conversation

AlecAivazis
Copy link
Owner

@AlecAivazis AlecAivazis commented Jun 1, 2019

This PR continues the work in #201 and adds the remaining AskOpts:

  • survey.WithHelpInput to change the rune that prompts look for to provide help.
  • survey.WithIcons to change the icons
  • survey.WithFilter to change the default filter applied to Select/MultiSelect

This has to go in after #214 since it relies on the PromptConfig which was introduced it that PR.

@AlecAivazis AlecAivazis requested a review from coryb June 1, 2019 03:22
@AlecAivazis AlecAivazis changed the title Added WithHelpInputRune AskOpt Added WithHelpInputRune and WithIconSet AskOpts Jun 1, 2019
core/renderer.go Outdated Show resolved Hide resolved
@AlecAivazis AlecAivazis force-pushed the help-input-ask-opt branch from bf49040 to 61f9fcf Compare June 3, 2019 06:01
@AlecAivazis AlecAivazis force-pushed the help-input-ask-opt branch from 61f9fcf to a0e1633 Compare June 3, 2019 06:07
@AlecAivazis
Copy link
Owner Author

@coryb there is now parity across the methods in the Prompt interface. Not sure why I wanted to avoid another file in the root of the project when there are already so many...

@AlecAivazis
Copy link
Owner Author

AlecAivazis commented Jun 3, 2019

I thought a bit more about this API and i think one of two things needs to happen. Either I should remove WithHelpInput or HelpInput should be removed from the IconSet struct. It's not clear from the API what would happen if you passed both WithHelpInput and a WithIconSet that modified the icon since they modify the same field in the icon set.

Any preference @coryb? Keeping it in the IconSet has a smaller API surface since its kind of tucked away. But it's also not really an "icon" like the others since it's input and would never include any kind of color configuration regardless of how #141 shakes out.

EDIT: I decided to go with removing HelpInput from the IconSet. I think this is good to go now.

Copy link
Collaborator

@coryb coryb left a comment

Choose a reason for hiding this comment

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

looks good, I agree with the HelpInput change and moving the Icons to the Config struct.

@AlecAivazis AlecAivazis changed the title Added WithHelpInputRune and WithIconSet AskOpts Added missing AskOpts Jun 3, 2019
@AlecAivazis AlecAivazis merged commit 0b0e8ae into master Jun 3, 2019
@AlecAivazis AlecAivazis deleted the help-input-ask-opt branch June 3, 2019 22:12
siredmar pushed a commit to siredmar/survey that referenced this pull request Feb 20, 2020
* updated readme

* added ask opt to set help input rune

* removed global icon definitions

* templates refer to icon field

* added explicit icon sets to tests

* added missing icon set in test runner

* removed errant log

* added missing icon set in select test

* added WithIconSet AskOpt

* can set error icon

* Prompt interface now uses PromptConfig

* tests use default icon set instead of hardcoded value

* config comes first in prompt inteface

* ran gofmt

* renamed WithHelpInputRune to WithHelpInput

* removed HelpInput from icon set

* removed unnecessary type cast

* removed globally defined default icon set

* added AskOpt to set default filter

* documented WithFilter

* renamed WithIconSet to WithIcons
cben added a commit to cben/survey that referenced this pull request May 11, 2021
Have been disabled since AlecAivazis#216.

Co-authored-by: Boris Od <[email protected]>
AlecAivazis pushed a commit that referenced this pull request May 12, 2021
* Re-enable `TestAsk` tests.

Have been disabled since #216.

Co-authored-by: Boris Od <[email protected]>

* TestAsk: ensure `vi` is used in Editor tests

Co-authored-by: Boris Od <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants