wip: cdk selection #11770

amcdnl · 2018-06-12T21:34:22Z

WIP of CDK Selection Directive - Addresses #11309

Note: This PR includes 2 changes to Selection Model.

Change onChange event to change to honor standard
Add optional trackBy

d3lm · 2018-06-13T14:25:10Z

First things first, really great work on the CDK selection feature @amcdnl 👏

I was thinking whether it made sense to add also a drag-to-select feature to the CDK as part of this WIP. I have recently (a few weeks back) implemented a library called ngx-drag-to-select for this and people seem to like it and they also start using it in their apps. It also covers mobile use-cases and should provide you with everything you need to implement it in many different scenarios. Here's a demo page. I tried to focus heavily on performance and also ran some performance audits. The results were pretty good (~60 FPS).

@jelbourn What do you think about integrating this into the Angular CDK as part of the selection feature? I would be totally open to completely overhaul the source code and re-write it if necessary so that the source code satisfies the Angular coding guideline / style. There also still some room for improvement, especially when it comes to the API and how to use it.

Any thoughts?

amcdnl · 2018-06-13T16:30:33Z

@jelbourn - Updated API per the feedback.

jelbourn

First pass

jelbourn · 2018-06-12T21:36:24Z

src/cdk-experimental/selection/selection.ts

+export class CdkSelection<T> implements OnInit {
+
+  /** List of all active selections. */
+  @Input('cdkSelection') selections: T[] = [];


selectedItems ?

Should this be a Set instead of an array?

A large set of items might get expensive if you're doing a lot of array manipulation

I'm guessing the items aren't ordered

re: set - I suppose it could be but from a public API perspective that feels odd. I totally see the value of doing that internally though. Also, the API of the selection model class is a array so since this is more just directive bindings for that I was trying to honor that api. Let me know your thoughts.

jelbourn · 2018-06-12T21:38:09Z

src/cdk-experimental/selection/selection.ts

+   * - Multiple: Multiple items can be selected.
+   * - Modifier Multiple: Multiple items can be selected using modifier keys such as ctrl or shift.
+   */
+  @Input('cdkSelectionStrategy') strategy: 'single' | 'multiple' | 'modifierMultiple' = 'single';


Could potentially bake this into the selector, e.g. cdkMultiSelection

Might also want to just do single and multiple with the modifier thing as a separate option, something like requireModifierKey

Yup - already done - 2bd4b3a#diff-06a4dd49126d9cf2343782d2b7f01cacR28

jelbourn · 2018-06-12T21:44:37Z

src/cdk-experimental/selection/selection.ts

+  @Input('cdkSelectionTrackBy') trackBy: (model: T) => any;
+
+  /** Whether the selections can be cleared after selection. */
+  @Input('cdkSelectionClearable')


"Clear" makes me thing it will do the whole thing, maybe "deselect" or "toggle"

Yup - already done - https://github.com/angular/material2/pull/11770/files#diff-06a4dd49126d9cf2343782d2b7f01cacR37

jelbourn · 2018-06-12T21:48:10Z

src/cdk-experimental/selection/selection.ts

+  private _clearable: boolean = true;
+
+  /** The max number of selections that can be selected */
+  @Input('cdkSelectionMaxSelections')


cdkSelectionMaxSelected ?

jelbourn · 2018-06-12T21:48:35Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Returns whether the control is multi select or not. */
+  getIsMultiple() {


Just isMultiple()

jelbourn · 2018-06-14T17:53:50Z

src/cdk-experimental/selection/selection.ts

+
+  /** Deregister a control from the set. */
+  deregister(ctrl) {
+    const idx = this._set.indexOf(ctrl);


idx -> toggleIndex

jelbourn · 2018-06-14T17:53:55Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Deregister a control from the set. */
+  deregister(ctrl) {


ctrl -> toggle (here and elsewhere)

jelbourn · 2018-06-14T17:55:21Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Set the user selection for the element. */
+  setTextSelection(type: string) {


I don't quite follow why this is here

When I do SHIFT + CLICK I want to set the text selection on the entire parent directive area to a specified state.

There should be a comment with the explanation

jelbourn · 2018-06-14T17:56:03Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Update the selections given the arguments. */
+  private _setSelections(ctrl: any) {


This function looks like it could use some comments

How about Set the selection state given a toggle directive and its activated options.?

jelbourn · 2018-06-14T17:56:41Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Get the index of the given element in the dom tree relative to the other toggles. */
+  private _getIndex(sortedSet: any[], sourceCtrl: any) {


_getToggleIndex ?

amcdnl · 2018-06-15T14:52:10Z

@jelbourn - Updated per feedback.

jelbourn

Second pass

jelbourn · 2018-06-15T22:53:53Z

src/cdk-experimental/selection/selection-toggle.ts

+  /** The modifier that was invoked. */
+  modifier: 'shift' | 'meta' | null;
+
+  private _destroy = new Subject();


_destroy -> _destroyed

jelbourn · 2018-06-15T22:54:30Z

src/cdk-experimental/selection/selection-toggle.ts

+  private _destroy = new Subject();
+
+  constructor(
+    public elementRef: ElementRef,


Ack; it needs a user-facing JsDoc, then

jelbourn · 2018-06-15T22:54:52Z

src/cdk-experimental/selection/selection-toggle.ts

+  }
+
+  /** Invoke toggle on the parent selection directive. */
+  _onToggle() {


_onToggle -> _toggle() ?

jelbourn · 2018-06-19T23:17:27Z

src/cdk-experimental/selection/selection.md

@@ -0,0 +1,63 @@
+The `selection` package handles managing selection state on components


FYI, I'll do an editing pass on this after it's submitted since it's easier than doing back and forth review of longer copy.

jelbourn · 2018-06-19T23:17:43Z

src/cdk-experimental/selection/selection.spec.ts

+
+  constructor(public elementRef: ElementRef) {}
+
+  clickITem(id: string, modifier?: 'meta' | 'shift') {


clickItem

jelbourn · 2018-06-19T23:42:31Z

src/cdk-experimental/selection/selection.ts

+
+  /** Set the selection state given a toggle directive and its activated options. */
+  private _setSelections(toggle) {
+    const ctrls = this._getSortedSet();


ctrls -> either toggles or toggleControls (here and in all other places). I personally like toggleControls as being the most explicit.

jelbourn · 2018-06-19T23:43:20Z

src/cdk-experimental/selection/selection.ts

+  /** Set the selection state given a toggle directive and its activated options. */
+  private _setSelections(toggle) {
+    const ctrls = this._getSortedSet();
+    const index = this._getToggleIndex(ctrls, toggle);


Could index be more specific?

jelbourn · 2018-06-19T23:45:30Z

src/cdk-experimental/selection/selection.ts

+    const { modifier, model } = toggle;
+    const selections = this._selectionModel.selected;
+
+    if (this.mode === 'multiple' && modifier === 'shift') {


Could the body of this method be broken up into two other methods: _setMultipleSelections and _setsingleSelections? Right now it feels like it has a lot going on.

jelbourn · 2018-06-19T23:46:55Z

src/cdk-experimental/selection/selection.ts

+
+    if (Array.isArray(toggle.model)) {
+      if (this.mode === 'multiple' &&
+          (this.maxSelections === undefined ||


Could this condition be broken into a function?

Kinda feels like overkill IMO.

jelbourn · 2018-06-19T23:47:45Z

src/cdk-experimental/selection/selection.ts

+  }
+
+  /** Get the set of controls sorted by DOM order. */
+  private _getSortedSet(): any[] {


_getSortedSet doesn't seem accurate since it's not returning a Set

amcdnl · 2018-06-24T15:32:51Z

@jelbourn -

What do you think about making CTRL + A do select all?

Also the selection-list component has a compareWith input rather than a trackBy. Do you think we should update this to match that?

re: implementation on select list - All of the mixins don't have any arguments in the constructor. For this I need to inject a variety of things into the injector so I don't think we can do it cleanly. I think it would be a better candidate for inheritance or encapsulated composition. Thoughts?

jelbourn · 2018-06-26T18:31:44Z

We use compareWith in autocomplete, too, I think. We can have a separate discussion about whether we want to unify them.
I'm not sure about the select-all. The amount of code for a downstream user to add it is pretty low, no?
What we do for mixins that require some dependency is that the "seed" class is required to have those as properties, e.g. the color mixin depends on starting from a class that implements HasElementRef

jelbourn · 2018-06-26T18:37:42Z

src/cdk-experimental/selection/selection-toggle.ts

+    '[class.cdk-selection-selected]': 'selected',
+    '(mousedown)': '_setModifiers($event)',
+    '(click)': 'toggle()',
+    /** Right click should select item. */


Should be a // style comment ( /** */ is only for JsDoc)

ngbot · 2018-06-29T15:54:18Z

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

amcdnl · 2018-07-15T19:25:39Z

@jelbourn

I'm not sure about the select-all. The amount of code for a downstream user to add it is pretty low, no?

Thats fine. So we would be adding a selectAll and deselectAll?

What we do for mixins that require some dependency is that the "seed" class is required to have those as properties, e.g. the color mixin depends on starting from a class that implements HasElementRef

I was playing with making this a mixin but the complexity exploded. I think it would be better as a higher order component.

jelbourn · 2018-07-16T21:17:39Z

src/cdk/collections/selection.ts

@@ -34,12 +34,13 @@ export class SelectionModel<T> {
  }

  /** Event emitted when the value has changed. */
-  onChange: Subject<SelectionChange<T>> | null = this._emitChanges ? new Subject() : null;
+  change: Subject<SelectionChange<T>> | null = this._emitChanges ? new Subject() : null;


Is this a breaking change?

Actually, I've already made this change in this PR and deprecated it - #8286 - I'm going to revert this back to the original name and we can consolidate on that merge :)

jelbourn · 2018-07-24T20:42:09Z

src/cdk/collections/selection.ts

@@ -39,7 +39,8 @@ export class SelectionModel<T> {
  constructor(
    private _multiple = false,
    initiallySelectedValues?: T[],
-    private _emitChanges = true) {
+    private _emitChanges = true,
+    private _trackBy?: (model) => any) {


Looks like there aren't any unit tests that cover this new trackBy behavior; this would be necessary since it's part of the main library rather than cdk-experimental

ngbot · 2018-08-21T18:47:19Z

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

ngbot · 2018-08-21T18:47:36Z

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

Adam-Michalski · 2020-05-27T05:47:50Z

@amcdnl this is great feature which is not yet merged. Can You make updates with changes from review?

angular-automatic-lock-bot · 2020-11-01T00:07:57Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

wip: cdk selection

b45d46f

amcdnl self-assigned this Jun 12, 2018

amcdnl requested a review from jelbourn June 12, 2018 21:34

amcdnl requested review from andrewseguin, crisbeto, devversion and tinayuangao as code owners June 12, 2018 21:34

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 12, 2018

chore: api feedback

2bd4b3a

amcdnl removed request for devversion, tinayuangao and andrewseguin June 13, 2018 16:30

jelbourn reviewed Jun 14, 2018

View reviewed changes

chore: PR feedback

f48d574

jelbourn reviewed Jun 19, 2018

View reviewed changes

amcdnl added 3 commits June 24, 2018 09:42

chore: nit

c515ae8

Merge remote-tracking branch 'origin/master' into cdk-selection

ce1561b

chore: hide public from docs

1a3f875

amcdnl added 4 commits June 24, 2018 10:33

chore: fix exports

59629aa

chore: fix type, add select all

913c434

chore: expose toggle api

b9e7261

chore: make space select

7888b82

jelbourn reviewed Jun 26, 2018

View reviewed changes

Merge branch 'master' into cdk-selection

81fd942

chore: fix nit

4749ee2

jelbourn reviewed Jul 16, 2018

View reviewed changes

amcdnl added 2 commits July 22, 2018 09:14

Merge remote-tracking branch 'upstream/master' into cdk-selection

f348bb5

chore: revert interface change

3fd4e4e

jelbourn reviewed Jul 24, 2018

View reviewed changes

mmalerba added aaa and removed aaa labels Apr 25, 2019

jelbourn closed this Oct 1, 2020

angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 1, 2020

		@@ -0,0 +1,63 @@
		The `selection` package handles managing selection state on components


		constructor(public elementRef: ElementRef) {}

		clickITem(id: string, modifier?: 'meta' \| 'shift') {

wip: cdk selection #11770

wip: cdk selection #11770

Uh oh!

Conversation

amcdnl commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

d3lm commented Jun 13, 2018

Uh oh!

amcdnl commented Jun 13, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl Jun 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Jun 15, 2018

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcdnl commented Jun 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jelbourn commented Jun 26, 2018

Uh oh!

amcdnl commented Jun 12, 2018 •

edited

Loading

amcdnl Jun 15, 2018 •

edited

Loading

amcdnl commented Jun 24, 2018 •

edited

Loading