feat: add a common class to be used when dealing with selection logic #2562
Conversation
Adds the `MdSelectionModel` class that can be used when dealing with single and multiple selection within a component. Relates to angular#2412.
googlebot
added
the
cla: yes
| * Class to be used to power selecting one or more options from a list. | ||
| * @docs-private | ||
| */ | ||
| export class MdSelectionModel { |
There was a problem hiding this comment.
Let's omit the Md prefix, since this will be part of the cdk. I'd also be okay with just calling the class Selection (with corresponding SelectionChange) (I do want to avoid prefixing any cdk classes with Cdk)
| * Class to be used to power selecting one or more options from a list. | ||
| * @docs-private | ||
| */ | ||
| export class MdSelectionModel { |
There was a problem hiding this comment.
We should be able to make the class generic:
export class SelectionModel<T> { ... }And then any properties dealing with the item are of type T (or arrays as T[])
| private _unflushedDeselectedValues: any[] = []; | ||
|
|
||
| /** Keeps track of the selected option that haven't been emitted by the change event. */ | ||
| private _unflushedSelectedValues: any[] = []; |
There was a problem hiding this comment.
Move all of these properties above the constructor
| private _selectedValues: any[] = []; | ||
|
|
||
| /** Keeps track of the deselected options that haven't been emitted by the change event. */ | ||
| private _unflushedDeselectedValues: any[] = []; |
There was a problem hiding this comment.
Not sure "unflushed" is the right term here. How about something like "pendingDeselection", "deselectedToEmit"?
| onChange: Subject<MdSelectionChange> = new Subject(); | ||
|
|
||
| /** Currently-selected values. */ | ||
| private _selectedValues: any[] = []; |
There was a problem hiding this comment.
Rather than keeping an array of selectedValues, we should keep the selection in a Set (named _selection). We can use .keys() when we want the list of selected values (caching the value). This will make selecting, deselecting, and checking selection much faster (and the code simpler). When the options are updated, the update would then look like.
Overall this would change insertion, deletion from O(k) to O(1) and keep getting an array of the selected values mostly the same.
| */ | ||
| export class MdSelectionModel { | ||
| constructor( | ||
| private _options: any[], |
There was a problem hiding this comment.
I think we can avoid keeping _options in this model completely if we add it as an argument to selectAll. That would make this class much simpler (no update logic, remove _verifyExistence)
|
Re-did it based on the feedback @jelbourn. |
| /** | ||
| * Selects a value or an array of values. | ||
| */ | ||
| select(value: T): void { |
There was a problem hiding this comment.
I think it's a bit confusing to have select and _select methods that do different things. Maybe make the private one something like _markSelected? (same for deselect and clear)
There was a problem hiding this comment.
I'd say it's probably better to move the clearing of the selected value to _select instead. That way the only difference is whether the event is emitted.
There was a problem hiding this comment.
I still think, though, that having a select and _select method is something we should avoid. It would be very easy to call the wrong one.
There was a problem hiding this comment.
_markSelected makes sense, but should I rename _deselect and _clear in that case as well?
There was a problem hiding this comment.
Changed up the names so they're not easily confused with the public ones.
|
LGTM |
jelbourn
added
pr: lgtm
action: merge
|
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. |
Adds the
MdSelectionModelclass that can be used when dealing with single and multiple selection within a component.Relates to #2412.