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

Add selection prompt #33

Closed
wants to merge 7 commits into from

Conversation

erikgeiser
Copy link

@erikgeiser erikgeiser commented Nov 28, 2020

This PR adds a selection prompt with the following features:

Examples:
asciicast
asciicast

@meowgorithm
Copy link
Member

Thanks for the PR! At first glance this looks pretty good. Could you add an example application in a subdirectory (perhaps called example)?

@erikgeiser
Copy link
Author

erikgeiser commented Nov 29, 2020

My bad, I forgot to add the asciinema recordings I made for #25.

There's one concern I have before merging. This prompt supports custom key mappings, and a default key map is used when instantiating it via NewModel(). However, if a user manually instantiates it (m := selection.Model{}) it will not react to user input at all when no key map is set and the program has to be killed. Is that a user error or should I add some logic to detect whether the bare minimum set of keys is defined in the key map (maybe up, down, select). If not it would throw an error on startup.

Now, when writing this down, I think I should definitely do this.

@erikgeiser
Copy link
Author

Another consideration: Should I change the NewModel signature to take the choices as an argument? Currently, I chose the NewModel signature without arguments to be in line with the other models in this package. However, given that the choices are obviously mandatory it would make sense to take it as an argument such that NewModel returns a working prompt.

@meowgorithm
Copy link
Member

Yes, that makes sense; so maybe something like func NewModel(choices ...[]string) to keep it light. Another option might be to use functional arguments to allow for different ways of constructing the choices:

m := NewModel(WithChoices("a", b"))

This would also allow other options such as:

m := NewModel(
    WithChoices(c),
    WithKeybindings(k),
)

@erikgeiser
Copy link
Author

erikgeiser commented Nov 30, 2020

I would have used NewModel(choices []*Choices). This would then be used like this: NewModel(SliceChoices(mySlice)). This would cover all use cases and not just strings. In my opinion this is important because often the choices are arbitrary objects like in the custom example (see also second video above):

choices := []Article{
    {ID: "123", Name: "Article A"},
    {ID: "234", Name: "Article B"},
    {ID: "345", Name: "Article C"},
    {ID: "456", Name: "Article D"},
    {ID: "567", Name: "Article E"},
}

prompt := selection.NewModel(selection.Choices(choices))
prompt.Label = "Choose an article!"
prompt.PageSize = 3
prompt.Template = customTemplate
prompt.Filter = func(filter string, choice *selection.Choice) bool {
    article, _ := choice.Value.(Article)

    return strings.Contains(article.ID, filter)
}

I think that your second option would be too much given that the user can just set the optional configuration directly on the model. This also feels more in line with the other models. The choices would be taken as an argument only because they are mandatory.

@erikgeiser
Copy link
Author

Is there anything I can do or change to move forward with this?

@meowgorithm
Copy link
Member

@erikgeiser Sorry for the delay on this one; we've been really busy over here. It may be a little while until we can give this a proper review, but we will get to it.

@erikgeiser
Copy link
Author

Do you still plan to do something about this PR?

@meowgorithm
Copy link
Member

Hey Erik. We looked at this again, and while the component is a bit too specific for this repository, we really like it. Would you be open to creating a repository for it so we can link to it from the README as an additional resource?

@erikgeiser
Copy link
Author

Okay, I can work with that decision. I'll see if I can find the time to make a small prompt library around the selection prompt.

@meowgorithm
Copy link
Member

meowgorithm commented Jul 22, 2021

Sounds good. Please do let us know when it's ready.

I enjoyed reading your code, by the way. They notion of the key map you came up with was particularly impressive.

@erikgeiser
Copy link
Author

Thank you.

Ultimately, I think it is a good decision not to merge this into bubbles. Looking back I developed it to be directly used as a prompt by users while most other bubbles components are more like widgets that are intended to be used in larger models.

I took this opportunity to create a prompt library on top of bubbles containing this selection prompt:

/~https://github.com/erikgeiser/promptkit

@meowgorithm
Copy link
Member

It's great to see another release of promptkit and nice that the library works both standalone and as something that can be integrated into a larger Bubble Tea project.

I've added promptkit to the README.

@erikgeiser
Copy link
Author

Thank you, currently the library is fairly limited due to charmbracelet/bubbletea#24 as you can only run one prompt before key strokes are lost, but I'll see what I can due about that issue.

@meowgorithm
Copy link
Member

Ah, good point. It's an interesting issue and a tricky one to solve from what I understand. There are some notes in the thread around it: you're more than welcome to take a look.

@muesli
Copy link
Contributor

muesli commented Aug 11, 2021

Gonna close this one, as we resolved the situation. Thank you for all your amazing work, @erikgeiser!

@muesli muesli added the enhancement New feature or request label Aug 11, 2021
@muesli muesli closed this Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants