Skip to content

[Console] Enable arrow keys to browse matched autocomplete options #6564

Merged
merged 4 commits into from Jan 6, 2013

3 participants

@lmcd
lmcd commented Jan 5, 2013

See notes in original autocomplete pull request: #6391
See also @bamarni's pull request implementing more or less the same stuff: #6561

@fabpot
Symfony member
fabpot commented Jan 5, 2013

Looks good to me. Can you add some unit tests?

@bamarni
bamarni commented Jan 5, 2013

1 line addition... you definitely got me on the diff!

That's what I had mind too, excepted for the default highlight, but as you said it's usually displayed in the question so it 's not necessary. 👍

@bamarni bamarni and 1 other commented on an outdated diff Jan 5, 2013
src/Symfony/Component/Console/Helper/DialogHelper.php
- foreach ($autocomplete as $j => $value) {
- // Get a substring of the current autocomplete item based on number of chars typed (e.g. AcmeDemoBundle = Acme)
- $matchTest = substr($value, 0, $i);
+ foreach ($autocomplete as $value) {
+ // Get a substring of the current autocomplete item based on number of chars typed (e.g. AcmeDemoBundle = Acme)
+ $matchTest = substr($value, 0, $i);
@bamarni
bamarni added a note Jan 5, 2013

it can even be a 0 liner diff if you use strpos on the condition below instead of this :)

@lmcd
lmcd added a note Jan 6, 2013

Ha, good catch! I was on autopilot when I wrote this... how it wound up at no extra lines I do not know :p I guess it was less lines before I replaced all count($matches) with $numMatches

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@lmcd
lmcd commented Jan 6, 2013

@fabpot Added tests

@fabpot
Symfony member
fabpot commented Jan 6, 2013

merged, thanks. Can you add a note about this in the docs?

@fabpot fabpot added a commit that referenced this pull request Jan 6, 2013
@fabpot fabpot merged branch lmcd/autocomplete-arrows-new (PR #6564)
This PR was merged into the master branch.

Commits
-------

2b73975 Add new tests and fix problem with the second option being chosen on down arrow
8a0bcfb Use strpos in place of substr
9864d95 Not much use in blanking the array each time
a4d711c Enable arrow keys to browse matched autocomplete options

Discussion
----------

[Console] Enable arrow keys to browse matched autocomplete options

See notes in original autocomplete pull request: #6391
See also @bamarni's pull request implementing more or less the same stuff: #6561

---------------------------------------------------------------------------

by fabpot at 2013-01-05T10:12:47Z

Looks good to me. Can you add some unit tests?

---------------------------------------------------------------------------

by bamarni at 2013-01-05T12:51:51Z

1 line addition... you definitely got me on the diff!

That's what I had mind too, excepted for the default highlight, but as you said it's usually displayed in the question so it 's not necessary. 👍

---------------------------------------------------------------------------

by lmcd at 2013-01-06T04:07:02Z

@fabpot Added tests
64bf80c
@fabpot fabpot merged commit 2b73975 into symfony:master Jan 6, 2013

1 check passed

Details default The Travis build passed
@mmucklo mmucklo pushed a commit that referenced this pull request May 23, 2013
@fabpot fabpot merged branch lmcd/autocomplete-arrows-new (PR #6564)
This PR was merged into the master branch.

Commits
-------

2b73975 Add new tests and fix problem with the second option being chosen on down arrow
8a0bcfb Use strpos in place of substr
9864d95 Not much use in blanking the array each time
a4d711c Enable arrow keys to browse matched autocomplete options

Discussion
----------

[Console] Enable arrow keys to browse matched autocomplete options

See notes in original autocomplete pull request: #6391
See also @bamarni's pull request implementing more or less the same stuff: #6561

---------------------------------------------------------------------------

by fabpot at 2013-01-05T10:12:47Z

Looks good to me. Can you add some unit tests?

---------------------------------------------------------------------------

by bamarni at 2013-01-05T12:51:51Z

1 line addition... you definitely got me on the diff!

That's what I had mind too, excepted for the default highlight, but as you said it's usually displayed in the question so it 's not necessary. 👍

---------------------------------------------------------------------------

by lmcd at 2013-01-06T04:07:02Z

@fabpot Added tests
77aa7a8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.