Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix not highlighting selected items after scrolling on multisearch #130

Closed

Conversation

macocci7
Copy link
Contributor

@macocci7 macocci7 commented Apr 7, 2024

This PR fixes the bug below.
Laravel\Prompts\multisearch cannot highlight the items after scrolling.
Because array_is_list() in line 118 of Laravel\Prompts\Themes\Default\MultiSearchPromptRenderer returns false after scrolling, and $selected results in false.

For example, with the list of search result(and scroll: 5 option):

[
    0 => 'item1',
    1 => 'item2',
    2 => 'item3',
    3 => 'item4',
    4 => 'item5',
    5 => 'item6',
]

$prompt->visible() returns an array as follows before scrolling:

[
    0 => 'item1',
    1 => 'item2',
    2 => 'item3',
    3 => 'item4',
    4 => 'item5',
]

On the other hand, $prompt->visible() returns an array as follows after scrolling:

[
    1 => 'item2',
    2 => 'item3',
    3 => 'item4',
    4 => 'item5',
    5 => 'item6',
]

With this array, array_is_list() returns false.

I'm not sure if this is the right way,
but I created isConsecutive() method in Laravel\Prompts\Themes\Default\Concerns\Number trait instead of array_is_list().

* fixed isConsecutive() not working correctly with hash array
@taylorotwell taylorotwell marked this pull request as draft April 7, 2024 17:32
@@ -115,7 +116,7 @@ protected function renderOptions(MultiSearchPrompt $prompt): string
->map(function ($label, $key) use ($prompt) {
$index = array_search($key, array_keys($prompt->matches()));
$active = $index === $prompt->highlighted;
$selected = array_is_list($prompt->visible())
$selected = $this->isConsecutive(array_keys($prompt->visible()))
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR! I'm able to replicate the issue.

I'm worried about just checking for consecutive keys to determine whether we're capturing keys or values.

Imagine you're feeding database results into the multisearch where the keys are integer database IDs, and you want the key to be returned.

You may get consecutive keys for some search queries and not others, but you should never get a list (assuming the database indexes from 1).

I'd suggest the following instead:

Suggested change
$selected = $this->isConsecutive(array_keys($prompt->visible()))
$selected = array_is_list($prompt->matches())

The caveat with multisearch is that the options callback must consistently always return a list or never return a list. If you want the prompt to return the value instead of the key, the callback would need to use array_values (or the values collection method) after any filtering to ensure a list is always returned.

@jessarcher
Copy link
Member

Hey @macocci7,

I discovered some additional issues with the multisearch prompt when I was investigating your issue. I've created #132 to hopefully solve everything, including the issue I raised in the comment above. Going to close this one but feel free to let me know if you think I've missed something!

@jessarcher jessarcher closed this Apr 12, 2024
@macocci7
Copy link
Contributor Author

Hey @jessarcher ,

OK. I'll check out #132 later.

@macocci7 macocci7 deleted the fix/highlight-selected-after-scrolling branch April 14, 2024 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants