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

feat(material): add select all option to multiselect #1552

Merged
merged 1 commit into from
Apr 26, 2019

Conversation

juristr
Copy link
Collaborator

@juristr juristr commented Apr 24, 2019

This PR implements the new feature described in #1516 .

  • add selectAllOption configuration
  • allow passing boolean or string value to selectAllOption. String will be used as label if provided
  • implement select all / deselect all
  • add some automated tests

@juristr
Copy link
Collaborator Author

juristr commented Apr 24, 2019

@aitboudad WIP pull request. The functionality should work as expected. It's just missing some tests. Early feedback welcome 😄.

@juristr juristr force-pushed the feat/multi-select-selectall branch from a0fd4bb to 5dce775 Compare April 24, 2019 21:57
@juristr juristr marked this pull request as ready for review April 24, 2019 21:57
@juristr juristr requested a review from aitboudad April 24, 2019 21:57
<mat-pseudo-checkbox class="mat-option-pseudo-checkbox"
[state]="state">
</mat-pseudo-checkbox>
{{ getSelectAllLabel() }}
Copy link
Member

Choose a reason for hiding this comment

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

I think we should enforce passing the selectAll label to avoid handling the boolean check, so I propose to use:

{{ to.selectAllOption }}

get value() { return this.formControl.value || []; }
get state() {
if (this.value.length > 0) {
return this.value.length !== this.formFieldControl.options.length
Copy link
Member

Choose a reason for hiding this comment

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

👍 for formFieldControl.options

);
}

getSelectAllLabel() {
Copy link
Member

Choose a reason for hiding this comment

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

this should be removed

description: 'Description',
required: true,
multiple: true,
selectAllOption: true,
Copy link
Member

Choose a reason for hiding this comment

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

pass string instead:

selectAllOption: 'Select All',

@juristr
Copy link
Collaborator Author

juristr commented Apr 24, 2019

Gonna fix them tomorrow 👍 (ehm..later today actually 😅, it's 00:53am here)

@juristr juristr force-pushed the feat/multi-select-selectall branch from 5dce775 to c32e9d2 Compare April 25, 2019 19:24
@juristr juristr requested a review from aitboudad April 25, 2019 19:39
@juristr
Copy link
Collaborator Author

juristr commented Apr 25, 2019

Alright, applied the necessary changes and added a test to verify the option label is properly set 👍

@aitboudad
Copy link
Member

aitboudad commented Apr 25, 2019

LGTM, I let you merge this PR! 🎉🎉

@juristr juristr force-pushed the feat/multi-select-selectall branch from c32e9d2 to a4708ac Compare April 26, 2019 09:03
@juristr juristr merged commit 144f5fa into ngx-formly:master Apr 26, 2019
@juristr juristr deleted the feat/multi-select-selectall branch April 26, 2019 09:18
toggleSelectAll() {
this.formControl.setValue(
this.value.length !== this.formFieldControl.options.length
? this.formFieldControl.options.map(x => x.value)
Copy link

Choose a reason for hiding this comment

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

This doesn't work with option groups.
I think that would work:

this.formFieldControl.options.map(
  x => (x.group ? x.group.map(grp => grp.value) : x.value)
).flat()

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@fontsie yep in fact it doesn't . Can u open a new issue. We should work on a PR to fix it

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants