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

[AlecAivazis/survey#96] add support for tab completion on input #304

Conversation

lucassabreu
Copy link
Contributor

@lucassabreu lucassabreu commented Oct 6, 2020

related to #96

@lucassabreu lucassabreu force-pushed the issue/96/add-support-for-tab-completion-on-input branch 2 times, most recently from d77c260 to ee6c409 Compare October 9, 2020 11:43
@lucassabreu lucassabreu marked this pull request as ready for review October 9, 2020 12:12
@lucassabreu
Copy link
Contributor Author

i will create a new section similar to "filtering options" on how to use the new feature. but the code i think is done

examples/inputfilesuggestion.go Outdated Show resolved Hide resolved
input.go Outdated Show resolved Hide resolved
@lucassabreu lucassabreu force-pushed the issue/96/add-support-for-tab-completion-on-input branch from 7f1920a to 5ac2b87 Compare October 21, 2020 11:48
input.go Outdated Show resolved Hide resolved
@lucassabreu lucassabreu force-pushed the issue/96/add-support-for-tab-completion-on-input branch from a26a157 to 3b732c7 Compare October 23, 2020 10:04
Copy link
Owner

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

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

Sorry it took so long for me to pull this down and take a serious look at it. Life was insanely busy but I found some time.

So in general, I LOVE this feature. On top of the killer feature, you took all of the time to add all of the different bits to get this through - awesome!

I've run it on cmd.exe and the ubuntu subsystem on windows and it runs like a charm. One minor request - can you make tab cycle to the next item (as well as down arrow)? This reflects the way that screen readers interact with consoles so we can be more accesible.

@lucassabreu
Copy link
Contributor Author

thank you @AlecAivazis, i am glad you liked it.

i implemented the tab to go to next item for the suggestions now.

i also realized the select and multiselect does not have this behavior and will implement on then.

Copy link
Owner

@AlecAivazis AlecAivazis left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for adding that! I think this is finally good to go. Sorry again it took so long!

@AlecAivazis AlecAivazis merged commit 52436f4 into AlecAivazis:master Nov 2, 2020
@lucassabreu lucassabreu deleted the issue/96/add-support-for-tab-completion-on-input branch November 2, 2020 21:45
@AlecAivazis
Copy link
Owner

AlecAivazis commented Jun 28, 2021

@lucassabreu this PR seems to have broken the ability for the user to use arrow keys in order to navigate the input string :( I'd like to bring that functionality back, do you mind giving it a shot? If not, I can try to find some time

@lucassabreu
Copy link
Contributor Author

hi @AlecAivazis , I can look into it, but can you explain how to reproduce it? is just about moving the cursor on the input?

@AlecAivazis
Copy link
Owner

awesome! If you run any of the tests in input.go you'll see what i mean if you type a few characters and then try to press an arrow key to move left or right.

@AlecAivazis
Copy link
Owner

The issue is because input calls RuneReader.ReadRune instead of ReadLine which handles new characters inserted at arbitrary places in the input.

@AlecAivazis
Copy link
Owner

AlecAivazis commented Jun 28, 2021

I tried to sneak in some time this morning to get started on this but have to get back to work and might not have time to see it through to the end for a little. Here is my branch if you want to take a look (feel free to ignore it entirely if you have a better approach). The input and help tests work but I haven't tried adding any suggestions yet. We probably want to add an example with suggestions to the test suite so we can ensure the functionality stays.

@lucassabreu
Copy link
Contributor Author

right i will try to continue from the branch. i think i left some tests... but will try to add more if needed.

@AlecAivazis
Copy link
Owner

Thanks for picking this up! I really appreciate it

@lucassabreu
Copy link
Contributor Author

trying to fix in the pr #361

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.

None yet

3 participants