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.

Sign up for GitHub

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
wants to merge 2 commits into from
Closed

Fix not highlighting selected items after scrolling on multisearch #130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4cd0c97
Fix highlighting selected items after scrolling
macocci7 Apr 7, 2024
2552009
Fixed isConsecutive()
macocci7 Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions src/Themes/Default/Concerns/Number.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
<?php

namespace Laravel\Prompts\Themes\Default\Concerns;

Trait Number
{
/**
* Determine whether the values are consecutive integers.
*
* @param array<int|string, int> $values
* @return bool
*/
protected function isConsecutive(array $values): bool
{
return count($values) > 0 && array_filter($values, 'is_int') === $values
? range(
array_slice($values, 0, 1)[0],
array_slice($values, 0, 1)[0] + count($values) - 1
) === array_values($values)
: false;
}
}
3 changes: 2 additions & 1 deletion src/Themes/Default/MultiSearchPromptRenderer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ class MultiSearchPromptRenderer extends Renderer implements Scrolling
{
use Concerns\DrawsBoxes;
use Concerns\DrawsScrollbars;
use Concerns\Number;

/**
* Render the suggest prompt.
Expand Down Expand Up @@ -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.

? in_array($label, $prompt->value())
: in_array($key, $prompt->value());

Expand Down
100 changes: 100 additions & 0 deletions tests/Feature/MultiSearchPromptTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,106 @@
],
]);

it('supports scrolled results', function ($options, $expected) {
Prompt::fake([
'B', // Search for "Blue"
Key::UP, // Highlight "Blue"
Key::SPACE, // Select "Blue"
Key::UP, // Highlight "Blue"
Key::UP, // Highlight "Blue"
Key::SPACE, // Select "Blue"
Key::BACKSPACE, // Clear search
'G', // Search for "Green"
Key::UP, // Highlight "Green"
Key::SPACE, // Select "Green"
Key::UP, // Highlight "Green"
Key::UP, // Highlight "Green"
Key::SPACE, // Select "Green"
Key::BACKSPACE, // Clear search
Key::ENTER, // Confirm selection
]);

$result = multisearch(
label: 'What are your favorite colors?',
placeholder: 'Search...',
options: $options,
);

Prompt::assertStrippedOutputContains(<<<'OUTPUT'
┌ What are your favorite colors? ──────────────────────────────┐
│ Search... │
└────────────────────────────────────────────────── 0 selected ┘
OUTPUT);

Prompt::assertStrippedOutputContains(<<<'OUTPUT'
│ Search... │
├──────────────────────────────────────────────────────────────┤
│ ◼ Blue-900 │
│ ◼ Blue-700 │
│ ◼ Green-700 │
│ ◼ Green-500 │
└────────────────────────────────────────────────── 4 selected ┘
OUTPUT);

Prompt::assertStrippedOutputContains(<<<'OUTPUT'
┌ What are your favorite colors? ──────────────────────────────┐
│ Blue-900 │
│ Blue-700 │
│ Green-700 │
│ Green-500 │
└──────────────────────────────────────────────────────────────┘
OUTPUT);

expect($result)->toBe($expected);
})->with([
'associative' => [
fn ($value) => strlen($value) > 0 ? collect([
'red' => 'Red',
'green-100' => 'Green-100',
'green-200' => 'Green-200',
'green-300' => 'Green-300',
'green-400' => 'Green-400',
'green-500' => 'Green-500',
'green-600' => 'Green-600',
'green-700' => 'Green-700',
'blue-100' => 'Blue-100',
'blue-200' => 'Blue-200',
'blue-300' => 'Blue-300',
'blue-400' => 'Blue-400',
'blue-500' => 'Blue-500',
'blue-600' => 'Blue-600',
'blue-700' => 'Blue-700',
'blue-800' => 'Blue-800',
'blue-900' => 'Blue-900',
])->filter(fn ($label) => str_contains(strtolower($label), strtolower($value)))->all() : [],
['blue-900', 'blue-700', 'green-700', 'green-500'],
],
'list' => [
fn ($value) => strlen($value) > 0 ? collect([
'Red',
'Green-100',
'Green-200',
'Green-300',
'Green-400',
'Green-500',
'Green-600',
'Green-700',
'Blue-100',
'Blue-200',
'Blue-300',
'Blue-400',
'Blue-500',
'Blue-600',
'Blue-700',
'Blue-800',
'Blue-900',
])->filter(fn ($label) => str_contains(strtolower($label), strtolower($value)))
->values()
->all() : [],
['Blue-900', 'Blue-700', 'Green-700', 'Green-500'],
],
]);

it('validates', function () {
Prompt::fake(['a', Key::DOWN, Key::SPACE, Key::ENTER, Key::DOWN, Key::SPACE, Key::ENTER]);

Expand Down
Loading