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

Select: add SelectAll feature for multi-selection mode #2642

Merged
merged 5 commits into from
Sep 5, 2021

Conversation

boukenka
Copy link
Contributor

@boukenka boukenka commented Sep 4, 2021

Here is the list of changes.

  • Property to add an item to select/unselect all items with SelectAll
  • Property to define the text of this option with SelectAllText
  • Property to define the delimited string with DelimitedStringSeparator

Remarks : A test has been added. The documentation has been updated with an example.

image

image

@boukenka boukenka marked this pull request as draft September 5, 2021 08:49
@boukenka boukenka marked this pull request as ready for review September 5, 2021 08:59
@boukenka
Copy link
Contributor Author

boukenka commented Sep 5, 2021

@henon Hello. Could you please do a review.

@rfrancisco
Copy link
Contributor

rfrancisco commented Sep 5, 2021

Would it make sense to have a divider between the selectAll menu option and the regular options to visually enforce that it is a special menu item? Or even have the selectAll menu item fixed (outside the scrolling area)

@henon
Copy link
Collaborator

henon commented Sep 5, 2021

@rfrancisco @boukenka we should look at other libraries (original Material UI, Vuetify, MUI, etc) for guidance

Comment on lines 419 to 429
protected async ValueTask SelectClearButtonClickHandlerAsync(MouseEventArgs e = default)
{
await SetValueAsync(default, false);
await SetTextAsync(default, false);
SelectedValues.Clear();
BeginValidate();
StateHasChanged();
await SelectedValuesChanged.InvokeAsync(SelectedValues);
await OnClearButtonClick.InvokeAsync(e);
if (e != default)
await OnClearButtonClick.InvokeAsync(e);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Refactor out a Clear() method (which you can make public, people will need that anyway) which is called by SelectClearButtonClickHandlerAsync, then you don't need the side-effect with default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactorisation has been done.

@boukenka
Copy link
Contributor Author

boukenka commented Sep 5, 2021

Would it make sense to have a divider between the selectAll menu option and the regular options to visually enforce that it is a special menu item? Or even have the selectAll menu item fixed (outside the scrolling area)

@rfrancisco I have added a separator.

xxx

@henon Here is an example found on https://v2.vuetifyjs.com/en/components/selects/

image

@@ -392,7 +416,7 @@ public override ValueTask SelectRangeAsync(int pos1, int pos2)
/// <summary>
/// Extra handler for clearing selection.
/// </summary>
protected async ValueTask SelectClearButtonClickHandlerAsync(MouseEventArgs e)
protected async ValueTask SelectClearButtonClickHandlerAsync(MouseEventArgs e = default)
{
await SetValueAsync(default, false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we call ClearAsync() here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad. I have removed the default. And as instructed, there is a now dedicated method ClearAsync.

@henon
Copy link
Collaborator

henon commented Sep 5, 2021

Make sure the spacing is even, like in Vuetify

image

Did you use a TriState checkbox for "Select all" which shows the undecided state (null) when only some are checked?
image

@boukenka
Copy link
Contributor Author

boukenka commented Sep 5, 2021

Make sure the spacing is even, like in Vuetify

@henon Please have a look to the screenshot below.

image

Did you use a TriState checkbox for "Select all" which shows the undecided state (null) when only some are checked?

No. This is a MudSelectItem component. There is now a TrisState checkbox (with an icon and some code for logic).

@henon
Copy link
Collaborator

henon commented Sep 5, 2021

Yesss! Looks great. Thanks you!

@henon henon merged commit ef210f1 into MudBlazor:dev Sep 5, 2021
@henon henon changed the title MudSelect : SelectAll, SelectAllText and DelimitedStringSeparator parameters Select: add SelectAll feature for multi-selection mode Sep 5, 2021
@henon henon added this to the 5.1.3 milestone Sep 5, 2021
@boukenka
Copy link
Contributor Author

boukenka commented Sep 5, 2021

@henon Thank you

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.

None yet

3 participants