feat(collections): add isMultipleSelection function to SelectionModel #11560
Conversation
If your component receives a SelectionModel instance through Input, it was previously not possible to have different behavior based on whether multiple selected items is allowed (e.g.: rendering radio buttons instead of checkboxes). You had to use a second Input property to the component. This pull request allows developers to check the SelectionModel instance property.
glenstaes
requested review from
andrewseguin,
crisbeto and
jelbourn
as code owners
googlebot
added
the
cla: yes
src/cdk/collections/selection.ts
Outdated
| @@ -111,7 +116,7 @@ export class SelectionModel<T> { | |||
| * Sorts the selected values based on a predicate function. | |||
| */ | |||
| sort(predicate?: (a: T, b: T) => number): void { | |||
| if (this._multiple && this._selected) { | |||
| if (this.multiple && this._selected) { | |||
There was a problem hiding this comment.
I think that for usage of the multiple value within the class that we can continue to just use the internal _multiple reference. This would leave the multiple value as a just a getter
There was a problem hiding this comment.
Do you want me to revert them? One could also argue that in the future the getter will have a more complex implementation so this change would make it future proof.
There was a problem hiding this comment.
For now, yes, it should reference the internal property. If the behavior changes then it could be updated
josephperrott
added
the
target: minor
src/cdk/collections/selection.ts
Outdated
| /** Whether multiple values can be selected. */ | ||
| get multiple() { | ||
| return this._multiple; | ||
| } |
There was a problem hiding this comment.
This should be a function isMultipleSelection instead of a getter; TypeScript generates more code for a getter, so it's preferrable to use a function if you aren't required to use a getter/setter (like with a directive binding)
src/cdk/collections/selection.ts
Outdated
| @@ -33,6 +33,11 @@ export class SelectionModel<T> { | |||
| return this._selected; | |||
| } | |||
|
|
|||
| /** Whether multiple values can be selected. */ | |||
| get multiple() { | |||
There was a problem hiding this comment.
There should be a unit test that runs against this public API
glenstaes
changed the title
|
Looks like there are a couple lint errors showing up. |
josephperrott
added
pr: lgtm
action: merge
src/cdk/collections/selection.ts
Outdated
| @@ -116,6 +116,13 @@ export class SelectionModel<T> { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Determines whether multiple values can be selected. | |||
There was a problem hiding this comment.
One last nit: the description should be "Gets whether multiple values can be selected."
|
If you can, could you push some new hash so that CircleCI tries to re-run? Would like it to be green before we merge |
glenstaes
changed the title
|
@andrewseguin All green now. |
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
If your component receives a SelectionModel instance through Input, it was previously not possible to have different behavior based on whether multiple selected items is allowed (e.g.: rendering radio buttons instead of checkboxes). You had to use a second Input property to the component. This pull request allows developers to check the SelectionModel isMultipleSelection function.