Skip to content

Add multisearch prompt #58

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

Merged
merged 17 commits into from
Sep 20, 2023
Merged

Add multisearch prompt #58

merged 17 commits into from
Sep 20, 2023

Conversation

irobin591
Copy link
Contributor

Hi there,

We have been using this package a lot at work, but we've run into one issue: sometimes we need to combine a search and a multiselect prompt.

Imagine the following scenario: The role association for a user is currently handled via console commands and consists of two steps: First one needs to search for a domain for which roles should be attached to a user, afterward one has to select none, one, or multiple roles related to the domain to attach to a user.

This results into the following prompts:

  • search prompt for selecting the user that should be modified
  • search prompt for a domain
  • multiselect prompt for specific roles within that domain with preselected roles that have already been given to the user

The search for a domain and the multiselect for roles might also be in a while loop, as one might want to add multiple roles on different domains.

Introducing: the multisearch prompt. This prompt allows searching for entries and selecting multiple of them, confirming everything with enter. When no search prompt has been entered, all currently selected entries are shown and can be deselected.

While working on the prompt, I noticed a bug which I have addressed in a separate commit: The scroll position is not being changed when updating a search. This might cause the list of entries to seem empty, if the list of entries is smaller than the current first visible index. You can still scroll, but it seems unintuitive.

@taylorotwell taylorotwell marked this pull request as draft September 5, 2023 13:47
@jessarcher
Copy link
Member

Nice work, @irobin591!

I've played with this locally and am not 100% sold on the behaviour though. If you select an option and then change the filter, it will hide the selected option, so it's unclear which options have been selected unless you go back and delete the search text.

It's tricky because there isn't a native HTML form element to base the UX on that people would be used to. The most common UI I've seen adds the selected options to the search input like this:

image

But this would be quite complicated to implement on the command line - especially allowing users to remove selections!

I wonder whether the selected options should always appear regardless of the search filter. Perhaps all together at the top or bottom of the dropdown, with any selected options filtered from the unselected results to prevent appearing twice.

What do you think?

@irobin591
Copy link
Contributor Author

Thanks for your feedback!

As you have noticed, I have implemented displaying all selected items when the search term is empty. But I see why this might not be the best solution, especially if there is no other indicator for this behaviour.
Adding a 5 items selected hint might be a solution for this, indicating that items have been selected.

I like your idea of adding all selected options at the top or bottom of the dropdown, but it might be cumbersome if many options are selected (i.e. via the default parameter) and the user wants to deselect a specific option. As all selected options are displayed and not affected by the search, this can become difficult to navigate.

If we want to always display all selected options, we can add another box either on top or below with all selected options, while still showing the filtered and potentially selected options in the search dropdown. 

@jessarcher
Copy link
Member

I like the "X items selected" idea. Or perhaps it could say "+X additional items" only when there are hidden items.

It could be integrated into the bottom border. E.g:

┌ Which users should receive the email? ───────────────────────┐
│ Foo                                                          │
├──────────────────────────────────────────────────────────────┤
│   ◼ Foo Bar                                                  │
└──────────────────────────────────────────── 5 items selected ┘


┌ Which users should receive the email? ───────────────────────┐
│ Foo                                                          │
├──────────────────────────────────────────────────────────────┤
│   ◼ Foo Bar                                                  │
└───────────────────────────────────────── +4 additional items ┘

@irobin591
Copy link
Contributor Author

Maybe even a combination of both as the amount of visible selected items might not be apparent as some might be not in view?

┌ Which users should receive the email? ───────────────────────┐
│ Foo                                                          │
├──────────────────────────────────────────────────────────────┤
│   ◼ Foo Bar                                                  │
└───────────────────────────────── 5 items selected (4 hidden) ┘

I'll work on these variants this weekend and see how it feels

@jessarcher
Copy link
Member

I just had a quick play around with the multiselect component, which would also benefit from something similar.

image

I can PR that, which would give you access to it as a new argument when drawing boxes.

@irobin591
Copy link
Contributor Author

irobin591 commented Sep 12, 2023

I just had a quick play around with the multiselect component, which would also benefit from something similar.

image

I can PR that, which would give you access to it as a new argument when drawing boxes.

Thanks for the PR, this seems perfect for this!

I was playing around with two different options: always displaying the count of hidden elements and only displaying it when something is hidden. I think always displaying the count of hidden elements fits better as we do not have a space issue and it already suggests, that the selected elements might sometimes be hidden:

"Hidden" is always visible:
multisearch-always-hidden

"Hidden" is only visible if the amount of hidden entries is greater than 0
multisearch-selective-hidden

@jessarcher
Copy link
Member

Nice! I think I prefer the second one, where it only shows the hidden count when there are hidden items. Feels a bit cleaner and ties in better with the multiselect prompt in that it only shows the extra information when it's relevant.

With the multiselect prompt I also decided not to display the count in the submit state because all of the selected items become visible. I think it would be good to keep that consistent here too.

@jessarcher
Copy link
Member

Just pushed a few changes, most notably to adopt the changes merged in #60.

I dig the allowKeys thing, but it felt better to invert it to ignore. Kinda wish I'd used this approach to prevent the enter key from submitting instead of the submit: false thing. Maybe in a breaking change release.

@jessarcher
Copy link
Member

jessarcher commented Sep 13, 2023

With the search prompt, it's possible to return items from the options callback when the search value is empty. I.e. an unfiltered list. It would be good to have that consistent here, but it currently only shows the selected items when the search value is empty.

A use case I'm thinking of would be for php artisan vendor:publish to let you publish multiple files in one go, with the search being effectively optional. E.g.:

$files = multisearch(
    label: "Which provider or tag's files would you like to publish?",
    placeholder: 'Search...',
    options: fn ($value) => collect([
        'Foo\ServiceProvider' => '<fg=gray>Provider:</> Foo\ServiceProvider',
        'Bar\ServiceProvider' => '<fg=gray>Provider:</> Bar\ServiceProvider',
        'foo-assets' => '<fg=gray>Tag:</> foo-assets',
        'foo-config' => '<fg=gray>Tag:</> foo-config',
        'bar-assets' => '<fg=gray>Tag:</> bar-assets',
        'bar-config' => '<fg=gray>Tag:</> bar-config',
    ])->when(
        strlen($value),
        fn ($vendor) => $vendor->filter(fn ($label) => str_contains(strtolower($label), strtolower($value)))
    )->all(),
);

@jessarcher
Copy link
Member

jessarcher commented Sep 13, 2023

To summarise my thoughts:

  • Remove the returnKeys parameter and instead return keys or values based on whether an associate array or list is returned from the options callback.
  • Remove the default option for now.
  • Allow the options callback to return items when the search value is empty.

In a separate PR, either of us could look at adding a default option to search and multisearch, ideally just passing an array of keys or values depending on whether you're returning an associate array or list from the options callback - any defaults would need to be included by the options callback when the search value is empty.

@jessarcher
Copy link
Member

jessarcher commented Sep 13, 2023

I've implemented the above requests. I'd appreciate your thoughts on the implementation, UX, and whether this still solves your use case.

@irobin591
Copy link
Contributor Author

Thanks for your review and implementation of your requests!

With the search prompt, it's possible to return items from the options callback when the search value is empty. I.e. an unfiltered list. It would be good to have that consistent here, but it currently only shows the selected items when the search value is empty.

I like your change adding the selected items on top when nothing has been selected! This way, the user can see all selected entries independent of what the developer has returned.

I just had the idea, whether we can provide the callable options a second argument containing the currently selected entries. This would give the developer more control on whether to always show the selected items and where these should be positioned, or to only show them when no search term has been provided?

In a separate PR, either of us could look at adding a default option to search and multisearch, ideally just passing an array of keys or values depending on whether you're returning an associate array or list from the options callback - any defaults would need to be included by the options callback when the search value is empty.

If it should be implemented like you described, it should be made clear to the developers, that an empty search value does not need to return all possible items, but at least the default items. Returning all possible items might cause issues with >1000 entries in the database. If we force the developer to load all entries from the database initially, there is no real difference between the suggest and the search prompt.

Remove the returnKeys parameter and instead return keys or values based on whether an associate array or list is returned from the options callback.

This change is for consistency with the search function, but this might cause issues if the options array is a list when nothing is filtered:

multisearch(
    label: 'Which users should receive the email?',
    placeholder: 'Search...',
    options: fn ($value) => collect([
        0 => 'User 0',
        1 => 'Test User 1',
        2 => 'User 2',
        3 => 'Test User 3',
        4 => 'User 4',
    ])->filter(
        fn ($label) => str_starts_with(strtolower($label), strtolower($value))
    )->all(),
);

multiselect-key-value-issue

This might happen when the list is fetched from the database with the index as key and the index starts at 0. This case should not be a common occurrence (MySQL starts auto-incrementing indexes at 1 by default, so no issue here), but should be something to consider as the developer has no control on whether indexes, values, or a mix of both are returned.

Remove the default option for now.

The default option is something we need for our use case, as we want to be able to deselect previously selected options. But this can be implemented at a later date.

Other than that, everything looks fine to me. What are your thoughts on my comments?

@jessarcher
Copy link
Member

I just had the idea, whether we can provide the callable options a second argument containing the currently selected entries. This would give the developer more control on whether to always show the selected items and where these should be positioned, or to only show them when no search term has been provided?

Interesting idea! But I'm a little worried about the UI inconsistencies it might lead to. Could always be implemented in a custom prompt.

If it should be implemented like you described, it should be made clear to the developers, that an empty search value does not need to return all possible items, but at least the default items. Returning all possible items might cause issues with >1000 entries in the database.

Yeah, it would need some documentation if we add that.

This change is for consistency with the search function, but this might cause issues if the options array is a list when nothing is filtered:

Yeah, that could be a bit of a foot gun. If you're working with a list, you'd need to make sure to always return a list after filtering (i.e. array_values() or $collection->values()). The same is already true with the search prompt. Probably worth a warning in the docs.

The default option is something we need for our use case, as we want to be able to deselect previously selected options. But this can be implemented at a later date.

Cool. Let's try a PR for that once this is merged.

@jessarcher
Copy link
Member

I'll fix the conflict once #70 is merged as it will conflict too.

@jessarcher
Copy link
Member

If this is merged, we'll need to PR a Windows fallback to laravel/framework.

I imagine it would be almost the same as the search fallback, which prompts for the search query and then prompts with the matching options for selection. The only difference is this would allow multiple selections.

@jessarcher jessarcher marked this pull request as ready for review September 18, 2023 01:10
@taylorotwell taylorotwell merged commit 4a98c05 into laravel:main Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants