add the ability to specify keys to select list items #152
Conversation
This change updates List (Prompt#select) to add the ability to specify keypress triggers for choices. It makes List respect the already-existing Choice#key, as well as adding displaying of the keys with :enum, overriding of those displayed keys with Choice#key_name, and modification of keypress behavior for List. The default behavior of keypress == move to item is kept, but the ability to have a keypress immediately select an item was added. This changeset also includes some whitespace, comment, and spec fixups.
|
Hi @piotrmurach, is there anything you'd like to see changed about this PR? |
|
Thank you for reminding me and sorry for keeping your waiting so long! Will review shortly. |
| # The text to display when this choice is used with :enum (i.e. in List) | ||
| # Defaults to :key. Use `nil` or `false` to disable showing the enum for | ||
| # this Choice. | ||
| # | ||
| # @api public | ||
| attr_reader :key_name |
There was a problem hiding this comment.
In light of the previous comment about :enum option, this comment would potentially need to change to reflect the changes
| #### 2.6.2.3 `:key_action` | ||
|
|
||
| When using `:enum` or Choice `:key` settings, you can change the keypress behavior. By default, `:key_action` is `:move`: pressing a number key or corresponding `:key` will move the cursor to select the choice. You can also use `key_action: :select` to make a keypress immediately select the item. | ||
|
|
||
|
|
||
| ```ruby | ||
| choices = [{name: "small", key: "s"}, {name: "medium", key: "m"}, {name: "large", key: "l"}] | ||
| prompt.select("What size?", choices, enum: ')', key_action: :select) | ||
| ``` | ||
|
|
||
| In the above example, when pressing "l", the "large" option will be immediately selected and the prompt will exit. |
There was a problem hiding this comment.
I like this concept, makes the selection even more powerful!
There was a problem hiding this comment.
Thank you for this PR! I appreciate the effort in putting this feature together.
After reviewing, my thoughts are that I'm unsure whether the idea of key selection fits well with the 'nature' of the select and multi_select prompts. I also wonder about the display and how 'intuitive' it will be for anyone using the prompt, especially in connection with :enum option? Would having keys appended at the end be a way to go? How would it work in multi_select prompt context? The :key_action only applies to select prompt as far as I can see.
There are few things going on in this PR and I feel that we could tease them apart to help move things forward. For example, some the changes and tests like the ones added to Choice and Choices I'd defo want to include and could be made in their own PR.
What do you think?
| # ‣ n) next | ||
| # p) previous | ||
| # esc) exit |
There was a problem hiding this comment.
I feel as though the :enum should still act as an enumerated list of items, even when combined with key selection. Potentially the display could resolve this by appending key afterwards like this:
# =>
# Do what? (Press ↑/↓ arrow to move and Enter to select)
# ‣ 1) next [n]
# 2) previous [p]
# 3) exit [esc]
There was a problem hiding this comment.
I agree, this could be side-by-side, and would likely be a lot more intuitive (i.e. you expect :enum to add the numbers, but for some reason :key overrides that)
There was a problem hiding this comment.
I agree that this can/should be split up into more manageable chunks. As far as display, the options I see are a) as you said, an appended [key] text, or b) underlining, like alt-key hotkey representation in many OSes. The second would be tricky in a situation where you want the :key to be a letter that is not in the word, or up/downcase version of it (i.e. fish, Fish, fiSh).
As for non-select usage, I had not considered that to be honest - it should definitely be supported, and I agree that :key_action => select really only makes sense for select. Maybe there can be a specific Menu class that works more like an interactive menu system, which is what I intended this for, versus adding that functionality into just select.
So, you'd like the changes split like this:
- A PR for the
Choice/Choiceschanges and related tests. - The ability to pres to move to an item in select
, and "check" item(s) inmulti_select` - The "interactive menu" stuff, i.e.
:key_action => select
If that all sounds good to you, I can get 1 & 2 done and we can talk more about 3.
There was a problem hiding this comment.
The a) suggestion seems to me a bit more straightforward in a sense that some keys don't directly correspond with letters like esc key. I like the idea of underlined letters but I worry about how the support looks in various terminals. Often the underline is not displayed at all. In general the b) option feels a bit more fragile to me.
There may be an opportunity to create some common menu abstraction. I definitely want to pursue an idea of splitting up the whole library into plugins. It would remain a single gem but all the menus would rely on common interfaces and hence provide a way for people to create their own prompts or enhance the current ones.
The plan sounds good to me. Could I be cheeky and suggest an item 0? When you made changes to slider prompt, you suggested that the default apart from index could accept a name. I like this idea and would extend it to select, multi_select and enum_select prompts as well. This could be then released with the slider changes. What do you think?
There was a problem hiding this comment.
In general the b) option feels a bit more fragile to me
Agreed. While it would be cooler looking, it definitely comes with a lot more complication. a) it is.
There may be an opportunity to create some common menu abstraction. I definitely want to pursue an idea of splitting up the whole library into plugins.
This would be neat. Making the "prompt" things more modular would be beneficial. If you need help with that, you know how to find me :)
extend it to select, multi_select and enum_select prompts as well
Ah, of course - I forgot about that! I will definitely do that as well!
There was a problem hiding this comment.
Hi Katelyn, it's been a while since we had our last discussion about this feature. In the meantime, I've updated the default option for all the prompts and made some changes to the Choice. I wonder if you have time to revisit adding key support again?
Describe the change
This change updates List (Prompt#select) to add the ability to specify keypress
triggers for choices. It makes List respect the already-existing Choice#key, as
well as adding displaying of the keys with :enum, overriding of those displayed
keys with Choice#key_name, and modification of keypress behavior for List. The
default behavior of keypress == move to item is kept, but the ability to have a
keypress immediately select an item was added. This changeset also includes some
whitespace, comment, and spec fixups.
Why are we doing this?
The readme called out this ability, but it didn't work with List. This adds the ability. Resolves #151
Benefits
This makes List more robust, especially in 'menu' style uses (i.e. simple keypresses to navigate menus, no arrow keys+enter keys needed)
Drawbacks
None immediately come to mind. I kept the original default behaviors, so this should be backwards-compatible.
Requirements
Put an X between brackets on each line if you have done the item:
[x] Tests written & passing locally?
[x] Code style checked?
[x] Rebased with
masterbranch?[x] Documentation updated?