search

laravel / prompts

Beautiful and user-friendly forms for your command-line PHP applications.
https://laravel.com/docs/prompts
MIT License
533 stars 94 forks source link

Support more readline keys #70

Closed storyn26383 closed 1 year ago

storyn26383 commented 1 year ago

With this PR, we can use:

see: https://github.com/laravel/prompts/pull/67#pullrequestreview-1623516659

jessarcher commented 1 year ago

Hey! I dig the CTRL_A/Home and CTRL_E/End stuff, especially with how little code it adds.

I'm not so sure about the usefulness of CTRL_U in prompts, or whether the complexity of CTRL_W is worth it. I'm also worried about going down a path where we reimplement everything from readline. I'm not sure if there are enough people who would use it to justify the added maintenance burden.

I'd rather see CTRL_A/Home and CTRL_E/End also scroll to the top and bottom of any lists when a list item is currently highlighted, but I think that is being worked on in #69.

storyn26383 commented 1 year ago

Yap, I agree with you, we should keep it clean and simple.

I'm also worried about going down a path where we reimplement everything from readline.

Use CTRL_A / Home and CTRL_E/ End to scroll to the top and bottom is cool! Maybe we can also add CTRL_B and CTRL_F to scroll page as well? (But this is vim's key binding 😛)

Should we implement scroll to the top and bottom feature in this PR, or open a new PR after #69 merged?

jessarcher commented 1 year ago

Thanks!

I just gave this a test, and the only issue is that the options callback on the search prompt gets called unnecessarily. I think you'd need to update the key listener of the search prompt so that it only sets highlighted to null instead of falling through to the default condition, which performs the search.

If we implement the home/end behaviour when focused on a list, we'll probably need to use the new ignore feature from #58 to conditionally make home/end only apply to the typed value when highlighted is null.

Should we implement scroll to the top and bottom feature in this PR, or open a new PR after #69 merged?

I'd hold off on that one until #58 and #69 are sorted as there are some overlaps and interdependent features.

If you can just fix the search callback I'll check it again and then hand it over to Taylor for final review.

storyn26383 commented 1 year ago

I think the suggest prompt should have the same behavior, so i fixed it too. Thanks for review :)

jessarcher commented 1 year ago

I think the suggest prompt should have the same behavior, so i fixed it too. Thanks for review :)

Good catch!

jessarcher commented 1 year ago

I've added docblocks to the CTRL_ constants so I can get my editor to remind me what they all are :sweat_smile: