Add ability to set hotkeys for multiselect #234

NickHeiner · 2019-11-09T00:33:31Z

I'd like the ability to customize the hotkey options available to the user for multiselect. My dream is to have something like git add -p, where you get hotkeys like:

Stage this hunk [y,n,q,a,d,e,?]?

Example usage:

await prompts({
      // ...
      hotkeys: {
        d: {
          handle() {
            /* handle user action */
          },
          instruction: 'Save current progress and stop answering questions.'
        }
      },
    })

This PR is a draft. If you like the idea, I'll add tests / docs.

NickHeiner · 2019-11-09T00:34:37Z

lib/elements/multiselect.js

@@ -155,6 +156,9 @@ class MultiselectPrompt extends Prompt {
      this.handleSpaceToggle();
    } else if (c === 'a') {
      this.toggleAll();
+    } else if (Object.keys(this.hotkeys).includes(c)) {
+      this.hotkeys[c].handle();
+      this.abort();


For my usecase, aborting was the correct behavior. If you'd like to move forward with this, I'll find a way to give the dev more control over what happens after the handler.

terkelg · 2019-11-09T03:14:58Z

This is cool – I like the idea a lot. Wonder if this could be a completely new prompt type?

NickHeiner · 2019-11-10T18:37:19Z

I think it could be a new prompt type, but at least for my use case, I'd also like to add it to multiselect. I'll add tests / docs to this PR next.

NickHeiner · 2019-11-11T22:47:53Z

lib/elements/multiselect.js

@@ -150,11 +151,27 @@ class MultiselectPrompt extends Prompt {
    this.render();
  }

-  _(c, key) {


This var was unused.

NickHeiner · 2019-11-11T22:49:25Z

readme.md

-  <img src="https://github.com/terkelg/prompts/raw/master/prompts.png" alt="Prompts" width="500" />
+  <img src="./prompts.png" alt="Prompts" width="500" />


This change:

Allows offline local rendering.

Improves portability if the repo were ever to move.

Makes the markdown more concise.

Nice one! How does this work with the readme shown on NPM? @lukeed you spent some time researching this right? I would be nice to have these readme updates in their own PR

The images still work on the npm registry page. For example, see: https://www.npmjs.com/package/list-maintainers.

I'm happy to split these changes out. I assume you only want the image href updates split out, but the docs for this feature should stay in this PR?

NickHeiner · 2019-11-11T22:51:36Z

test/multiselect.js

@@ -0,0 +1,125 @@
+const child_process = require('child_process');


To get more confidence than the existing tests provide, I created a test that spawns a child process and interacts with it as a user would.

I'm looking into how to test the lib for version 3. If you have more ideas for how to do testing, please let me know.

I think this file is a good test approach, and it can be expanded to the rest of the lib.

NickHeiner · 2019-11-11T22:53:04Z

This PR currently isn't passing on Node 6. (And there are a few cases, most notable lack of async/await support, that make supporting Node 6 make the code less readable.)

Node 6 is currently considered to be "end of life". It was released in 2016. How would you feel about dropping support for it?

NickHeiner · 2019-11-11T22:53:18Z

@terkelg how does this look to you now?

terkelg · 2019-11-13T15:04:26Z

@NickHeiner thank you for the PR! I'll have a look at this over the weekend. Quick note: You can use async/await. It's already used quite a few places

NickHeiner · 2019-11-13T17:49:52Z

.travis.yml

  - "8"
-  - "6"


Node 6 was already unsupported, because async/await was being used.

Can you revert this please 👍

NickHeiner · 2019-11-13T17:50:09Z

package.json

@@ -48,6 +48,6 @@
    "tape": "^4.11.0"
  },
  "engines": {
-    "node": ">= 6"
+    "node": ">= 7"


Node 6 was already unsupported, because async/await was being used. Thus, there's no need for a major version bump here.

Node 6 is still supported using Babel: https://github.com/terkelg/prompts/blob/master/index.js
https://github.com/terkelg/prompts/blob/master/package.json#L20

Ah, I didn't realize that. Ok. Well, Node 6 is EOL in any case. How would you feel about dropping support for it? My use of async / await will still require a bit more dev work to make the tests pass in Node 6.

It's a good idea to drop Node 6 support, but I don't think we should do that before v3

Very thought out PR. Thank you for paying attention to things like versioning. - appreciate it. However I have a script that automatically bump version and publish. Would you mind reverting this?

Yeah, for sure dropping Node 6 support should be behind a major version bump. And I take it you don't want this PR to be v3? 😄 I'll revert.

terkelg

Sorry for the late reply. I finally had time to look at your PR

terkelg · 2019-11-16T21:20:01Z

.travis.yml

  - "8"
-  - "6"


Can you revert this please 👍

terkelg · 2019-11-16T21:20:18Z

lib/elements/multiselect.js

@@ -150,11 +151,27 @@ class MultiselectPrompt extends Prompt {
    this.render();
  }

-  _(c, key) {


terkelg · 2019-11-16T21:22:59Z

package.json

@@ -48,6 +48,6 @@
    "tape": "^4.11.0"
  },
  "engines": {
-    "node": ">= 6"
+    "node": ">= 7"


Very thought out PR. Thank you for paying attention to things like versioning. - appreciate it. However I have a script that automatically bump version and publish. Would you mind reverting this?

terkelg · 2019-11-16T21:24:04Z

readme.md

-  <img src="https://github.com/terkelg/prompts/raw/master/prompts.png" alt="Prompts" width="500" />
+  <img src="./prompts.png" alt="Prompts" width="500" />


Nice one! How does this work with the readme shown on NPM? @lukeed you spent some time researching this right? I would be nice to have these readme updates in their own PR

terkelg · 2019-11-16T21:26:32Z

test/multiselect.js

@@ -0,0 +1,125 @@
+const child_process = require('child_process');


I'm looking into how to test the lib for version 3. If you have more ideas for how to do testing, please let me know.

This reverts commit 126c66f.

NickHeiner · 2019-11-17T00:50:47Z

Ok @terkelg I addressed your comments. It's now passing in Node 6.

NickHeiner · 2019-11-27T19:21:05Z

@terkelg I'm ready to move forward on this whenever you are. 😄

terkelg · 2019-11-28T15:12:48Z

Looks good. I wonder if we should pass some arguments to the handler callback? Like a reference to the prompts so that you can provide some sort of visual feedback?

NickHeiner · 2019-11-29T18:36:17Z

Looks good. I wonder if we should pass some arguments to the handler callback? Like a reference to the prompts so that you can provide some sort of visual feedback?

Sounds interesting, but I'm not sure precisely what you have in mind. Can you elaborate?

terkelg · 2019-11-30T21:29:40Z

Right now there's no feedback to the user to indicate a hotkey has been pressed/activated. I wonder if there could be a way to change selected mode in the hotkey callbacks?

NickHeiner · 2019-12-03T20:16:36Z

If a hotkey changes which answers are selected, that's reflected in the rendered output. In this gif, we can see that pressing a (which is a "built in hotkey") and r (which is a hotkey added by this PR) both provide the same type of feedback.

Does this address what you're looking for, or am I misunderstanding?

terkelg · 2019-12-05T14:07:21Z

I'll have a look at it this weekend – I didn't get that for some reason

terkelg · 2019-12-08T18:32:38Z

        {
            type: 'multiselect',
            name: 'multicolor',
            message: 'Pick colors',
            hint: false,
            hotkeys: {
                d: {
                    handle(x, y) {
                        /* handle user action */
                    },
                    instruction: 'Save current progress and stop answering questions.'
                }
            },
            choices: [
                { title: 'Red', description: 'This option has a description.', value: '#ff0000' },
                { title: 'Green', value: '#00ff00' },
                { title: 'Yellow', value: '#ffff00', disabled: true },
                { title: 'Blue', value: '#0000ff' }
            ]
        },

When I run this I get no visual feedback as a user that I hit the hotkey?

Add ability to set hotkeys for multiselect

67cd548

NickHeiner commented Nov 9, 2019

View reviewed changes

NickHeiner mentioned this pull request Nov 9, 2019

Interactive Mode facebook/jscodeshift#352

Open

Nick Heiner added 11 commits November 11, 2019 11:24

with great effort, and deep, deep concentration, a man can achieve

ff65bd4

Update travis

f06026a

Making the test better

f0d6f9b

Adding a failing test for having a hotkey that selects multiple answers

760b308

Make the test pass

626e4c5

Adding ability to abort

58d570f

Add a test for having a hotkey submit an answer

d08aff4

Add a test for deselecting an answer

59f9c27

README cleanup

aaaa5db

README

94bcfc1

update README

63561e0

NickHeiner commented Nov 11, 2019

View reviewed changes

Clean up tests

3725cab

NickHeiner commented Nov 11, 2019

View reviewed changes

Bump version

2cc9ee5

Embrace async/await

126c66f

NickHeiner commented Nov 13, 2019

View reviewed changes

terkelg requested changes Nov 16, 2019

View reviewed changes

Nick Heiner added 2 commits November 16, 2019 16:34

Revert unwanted changes

a831315

Revert "Embrace async/await"

27ed003

This reverts commit 126c66f.

Support node 6

eb38dc6

NickHeiner requested a review from terkelg November 27, 2019 19:21

		<img src="https://github.com/terkelg/prompts/raw/master/prompts.png" alt="Prompts" width="500" />
		<img src="./prompts.png" alt="Prompts" width="500" />

		@@ -0,0 +1,125 @@
		const child_process = require('child_process');

Add ability to set hotkeys for multiselect #234

Are you sure you want to change the base?

Add ability to set hotkeys for multiselect #234

Conversation

NickHeiner commented Nov 9, 2019 • edited Loading

Choose a reason for hiding this comment

terkelg commented Nov 9, 2019

NickHeiner commented Nov 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickHeiner commented Nov 11, 2019

NickHeiner commented Nov 11, 2019

terkelg commented Nov 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terkelg Nov 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

terkelg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickHeiner commented Nov 17, 2019

NickHeiner commented Nov 27, 2019

terkelg commented Nov 28, 2019

NickHeiner commented Nov 29, 2019

terkelg commented Nov 30, 2019

NickHeiner commented Dec 3, 2019

terkelg commented Dec 5, 2019

terkelg commented Dec 8, 2019

NickHeiner commented Nov 9, 2019 •

edited

Loading

terkelg Nov 13, 2019 •

edited

Loading