[HxMultiSelect] filtering and select all

[bholbrook]

Summary of changes

• Added filtering capability to the HxMultiSelect component as per Issue 616

• Added select all capability to the HxMultiSelect component as per Issue 453

Clicking when unchecked will select all items in the currently filtered list.
Clicking when checked will deselect all items in the currently filtered list.
Clicking a checked item or entering any text into the filter will uncheck the select all.
Manually clicking and selecting all items in the filtered list will not check the select all.

[hakenr]

I'm sorry, but I misunderstood the intent, we kind of missed each other. I thought we were talking about a new search field inside the dropdown and I didn't notice that you were describing using an existing input for filtering.
We would actually like to teach the component this UX:

In our opinion, the filtering functionality combined with the main input is confusing because the selected items are not visible when filtering.

I am very sorry, but we cannot accept the filtering functionality in this form. If you would still be willing to give your time to rework it into the intended form, we would be very grateful. Otherwise, we will try to move the request higher in the backlog to appreciate the energy put into the functionality and get the filtering into the component sooner.

[hakenr]

@bholbrook Is the PR ready for review now or are you still working on the code?

[bholbrook]

@hakenr It's ready to be re-reviewed when you can please :)

[bholbrook]

@hakenr I was just wondering if/when this might get a re-review please?

[hakenr]

@bholbrook Sorry for the delay. It is on my TODO list. Hopefully I will get to it today or tomorrow.

[hakenr]

@bholbrook First off, I have to commend the effort you've put into working on changes of this scale. I myself was surprised by the sheer amount of changes, which is partly due to the combination with adding the "select all" feature. This raises a lot of new questions, although I understand that implementing both features together is more efficient.

For now, I've added some inline code comments and have noted the following points (for myself) that need to be addressed. Most of these will require further thought on my part, and some will be discussed with colleagues in our internal design meeting:

• AllowFiltering, AllowSelectAll naming?
• EmptyTemplate naming. Perhaps FilterEmptyResultTemplate and FilterEmptyResultText (with defaults) for easier usage?
• SelectAll when filtered - should it select only visible items? Is that what we want?
• Filter-box - should it have a search icon and a clear icon?
• ClearFilterOnHide - should the default be true?
• OnShown and OnHidden - do we even need these parameters? How would anyone use them?
• MultiSelectSettings - AllowFiltering, AllowSelectAll, ClearFilterOnHide, ?FilterPredicate?
• SelectAllChanged - do we even need such an EventCallback? How would it be used? If so, maybe OnSelectAllChanged, and it should not have a value.
• Missing documentation for new parameters
• Missing demo documentation for new features
• Default "-- select all --" text - do we want those dashes? If so, probably just one on each side?
• ItemSelectionChanged - we probably can't use this for the SelectAll scenario, as it would trigger a series of ValueChanged for each selected item, whereas it might be more appropriate to trigger ValueChanged just once with a List<TValue> of all items
• InputSize to be applied to inner filter-input

Looking forward to your thoughts on these points.

[hakenr]

@crdo, this is a significant contribution that we shouldn't let sit idle. Could you please take a look at the PR (UX, HTML, API, ...) when you get a chance? Thanks!

[bholbrook]

@hakenr In response to your list, these are my thoughts.

• AllowFiltering, AllowSelectAll naming?
  I don't mind what these are called as long as it's obvious they are a boolean on/off feature using an action verb prefix.
  Happy to change them to whatever you want, but I'd rather have the functionality committed than a perfected name.

• EmptyTemplate naming. Perhaps FilterEmptyResultTemplate and FilterEmptyResultText (with defaults) for easier usage?
  Have renamed the existing template parameter to FilterEmptyResultTemplate.
  Have added a new FilterEmptyResultText parameter.
  Have added a default empty template utilising this new FilterEmptyResultText parameter. This also uses localisation.

• SelectAll when filtered - should it select only visible items? Is that what we want?
  I definitely think so. You have to actively filter and you have to have to actively click the select all button, which means when you do click select all you've actively chosen to select what you see contextually.
  I can't think of a single real-world scenario when you'd want to select every item in the list while you've got a filter.

• Filter-box - should it have a search icon and a clear icon?
  A search icon has been added and will be displayed when there's no filter text.
  A clear icon has been added and will be displayed when there is filter text.

• ClearFilterOnHide - should the default be true?
  I have changed the default to true.
  As a note: When the dropdown is expanded the filter is automatically highlighted so the user can just hit delete so even if they do actively set it to false the filter is still very usable.

• OnShown and OnHidden - do we even need these parameters? How would anyone use them?
  I have removed these.

• MultiSelectSettings - AllowFiltering, AllowSelectAll, ClearFilterOnHide, ?FilterPredicate?
  Added AllowFiltering and AllowSelectAll to the MultiSelectSettings and set both defaults to false.
  Added ClearFilterOnHide to the MultiSelectSettings and set the default to true.

• SelectAllChanged - do we even need such an EventCallback? How would it be used? If so, maybe OnSelectAllChanged, and it should not have a value.
  I have removed this.

• Missing documentation for new parameters
  All documentation has been added to HxMultiSelect and removed from HxMultiSelectInternal

• Missing demo documentation for new features
  I have added new examples for basic filtering, custom filtering, and a simple select all example.
  I don't know how to run the documentation project locally so haven't been able to test these :(
  How can I run the documentation to check please?

• Default "-- select all --" text - do we want those dashes? If so, probably just one on each side?
  I've changed this to default to "-select all-".

• ItemSelectionChanged - we probably can't use this for the SelectAll scenario, as it would trigger a series of ValueChanged for each selected item, whereas it might be more appropriate to trigger ValueChanged just once with a List<TValue> of all items
  I have reworked the callbacks as per recommendations.

[hakenr]

@bholbrook

SelectAll when filtered - should it select only visible items? Is that what we want?

I definitely think so. You have to actively filter and you have to have to actively click the select all button, which means when you do click select all you've actively chosen to select what you see contextually.
I can't think of a single real-world scenario when you'd want to select every item in the list while you've got a filter.

I agree that you don't want to "select all including those filtered out", but we should somehow express this in the UX (text?) when filtering is applied. Maybe something like "add filtered items to selection" (Huh, do we want to add the items to existing selection or do we want to replace the selection?).

Filter-box - should it have a search icon and a clear icon? [IN PROGRESS]

A search icon isn't required as it will update the list immediately as the user types.

This was meant as a visual indicator "this is the input for filtering" not as "spinner" while loading the data.

SelectAllChanged - do we even need such an EventCallback? How would it be used? If so, maybe OnSelectAllChanged, and it should not have a value.

I have removed this.

Have you pushed all the changes? It is still there with all the related code.

ItemSelectionChanged - we probably can't use this for the SelectAll scenario, as it would trigger a series of ValueChanged for each selected item, whereas it might be more appropriate to trigger ValueChanged just once with a List of all items

TBD

We probably have to change SelectionChangedArgs to allow multiple changes to be signaled at once. Probably something like

public class SelectionChangedArgs
{
	public List<TItem> ItemsDeselected { get; set; }
	public List<TItem> ItemsSelected { get; set; }
}

The HandleItemSelectionChanged can then change the CurrentValue accordingly.

[bholbrook]

@hakenr Haha, apparently I made the SelectAllChanged changes in my head! I've pushed them up now.

In terms of the SelectAll filtering language, I'm really not sure what to do here. If you have any suggestions I'm happy to make those changes.

I'm currently working on the Search/Clear icon. I didn't about having the Search icon there but that's not a bad idea.
I'll attempt to implement it similarly to the others whereby the Search icon will display while the filter box is empty and the Clear icon will display when there is any text.

And the ItemSelectionChanged stuff I haven't yet investigated. It's likely the last thing I'll work on.

[bholbrook]

@hakenr I've just pushed up the final changes to your list of suggestions.
With them, everything should be ready to go.
If there is anything you think I should change (outside of documentation because I don't know how to run that project) please do let me know and I'll gladly make those changes.
I hope to get this integrated soon!

Also, once this does go in, how long might it be before this gets deployed to NuGet and made available?

[hakenr]

LGTM.
Will do some minor tuning and will merge to master.