Skip to content

wip: cdk selection #11770

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

Closed
wants to merge 14 commits into from
Closed
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

49 changes: 49 additions & 0 deletions src/cdk-experimental/selection/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package(default_visibility=["//visibility:public"])
load("@angular//:index.bzl", "ng_module")
load("@build_bazel_rules_typescript//:defs.bzl", "ts_library", "ts_web_test")
load("@io_bazel_rules_sass//sass:sass.bzl", "sass_binary")


ng_module(
name = "selection",
srcs = glob(["**/*.ts"], exclude=["**/*.spec.ts"]),
module_name = "@angular/cdk-experimental/selection",
assets = [] + glob(["**/*.html"]),
deps = [
"//src/cdk/coercion",
"//src/cdk/collections",
"@rxjs",
],
tsconfig = "//src/cdk-experimental:tsconfig-build.json",
)


ts_library(
name = "selection_test_sources",
testonly = 1,
srcs = glob(["**/*.spec.ts"]),
deps = [
":selection",
"//src/cdk/collections",
"//src/cdk/testing",
"@rxjs",
],
tsconfig = "//src/cdk-experimental:tsconfig-build.json",
)

ts_web_test(
name = "unit_tests",
bootstrap = [
"//:web_test_bootstrap_scripts",
],
tags = ["manual"],

# Do not sort
deps = [
"//:tslib_bundle",
"//:angular_bundles",
"//:angular_test_bundles",
"//test:angular_test_init",
":selection_test_sources",
],
)
9 changes: 9 additions & 0 deletions src/cdk-experimental/selection/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

export * from './public-api';
Empty file.
18 changes: 18 additions & 0 deletions src/cdk-experimental/selection/selection-module.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import { NgModule } from '@angular/core';
import { CdkSelection } from './selection';
import { CdkSelectionToggle } from './selection-toggle';
Copy link
Member

Choose a reason for hiding this comment

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

Omit spaces in braces



@NgModule({
declarations: [CdkSelection, CdkSelectionToggle],
exports: [CdkSelection, CdkSelectionToggle]
})
export class CdkSelectionModule {}
Copy link
Member

Choose a reason for hiding this comment

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

When this is merged into the cdk for real, I think it would make sense to add it to the existing collections subpackage

109 changes: 109 additions & 0 deletions src/cdk-experimental/selection/selection-toggle.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

import {Directive, Input, OnInit, OnDestroy, ElementRef, ChangeDetectorRef} from '@angular/core';
import {CdkSelection} from './selection';
import {Subject} from 'rxjs';
import {takeUntil} from 'rxjs/operators';


@Directive({
selector: '[cdkSelectionToggle]',
host: {
'class': 'cdk-selection-toggle',
'role': 'option',
Copy link
Member

Choose a reason for hiding this comment

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

Don't want to force a role; something like table wouldn't want to be option

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, forgot to remove this.

'tabindex': '-1',
'[attr.aria-selected]': 'selected',
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this should use aria-selected, which isn't always the right thing; sometimes it's aria-checked or aria-pressed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, forgot to remove this.

'[class.cdk-selection-selected]': 'selected',
'(mousedown)': '_onMouseDown($event)',
'(click)': '_onToggle()',
'(contextmenu)': '_onToggle()',
Copy link
Member

Choose a reason for hiding this comment

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

Why contextmenu?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In things like Finder in mac, when you right click it selects the active item. I wanted it to follow that here.

Copy link
Member

Choose a reason for hiding this comment

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

Add comment with the explanation for contextmenu

'(keydown.enter)': '_onToggle()',
}
})
export class CdkSelectionToggle<T> implements OnInit, OnDestroy {

/** Model of the item to toggle selection. */
Copy link
Member

Choose a reason for hiding this comment

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

Expand comment to explain the distinction between a single value or an array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about The value(s) that represent the selection of this directive.

@Input('cdkSelectionToggle') model: T | T[];

/** Whether the selection is disabled or not. */
Copy link
Member

Choose a reason for hiding this comment

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

Don't need the "or not" at the end of the comment (here and other boolean descriptions)

@Input() disabled: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Use coerceBooleanProperty? (for this and other boolean properties)


/** Whether the toggle is selected or not. */
selected: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an @Input?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, similar to the sort you set the active states on the parent. This is just a internal state. Perhaps I should underscore it?


/** The modifier that was invoked. */
Copy link
Member

Choose a reason for hiding this comment

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

"that was invoked" when?

  • Should this be internal?
  • I don't quite follow why you need to remember the modifiers; don't you only care when the click occurs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my original implementation I listened for this separately because in cases like a checkbox I would listen to the change event rather than click and this information would not be available on that change event. I figured as we carry this forward to other implementations like the checkbox having this at the root would simplify those scenarios.

The modifier is public so the selection directive parent can read it out w/o having to pass it.

Let me know your thoughts here.

modifier: 'shift' | 'meta' | null;

private _destroy = new Subject();
Copy link
Member

Choose a reason for hiding this comment

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

_destroy -> _destroyed


constructor(
public elementRef: ElementRef,
Copy link
Member

Choose a reason for hiding this comment

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

Does the elementRef have to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, its used in the parent directive to identify the element order.

Copy link
Member

Choose a reason for hiding this comment

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

Ack; it needs a user-facing JsDoc, then

private _selection: CdkSelection<T>,
private _cd: ChangeDetectorRef
Copy link
Member

Choose a reason for hiding this comment

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

_changeDetectorRef

) {}

ngOnInit() {
this._selection.selectionChange
.pipe(takeUntil(this._destroy))
.subscribe(() => this._checkSelected());

this._checkSelected();
this._selection.register(this);
Copy link
Member

Choose a reason for hiding this comment

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

Why do this in onInit instead of the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Personally I try to keep the constructor to only setup functions and put event operations/etc on init. Let me know your thoughts.

}

ngOnDestroy() {
this._destroy.next();
this._destroy.complete();

this._selection.deregister(this);
}

/** Mousedown even to capture modifiers. */
_onMouseDown(event: MouseEvent) {
Copy link
Member

Choose a reason for hiding this comment

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

In general methods should be named for what they do rather than when they're called

this._setModifiers(event);
}

/** Invoke toggle on the parent selection directive. */
_onToggle() {
Copy link
Member

Choose a reason for hiding this comment

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

_onToggle -> _toggle() ?

if (!this.disabled) {
this._selection.toggle(this);
}
}

/** Set the modifiers for the mouse event. */
_setModifiers(event) {
if (event.metaKey || event.ctrlKey) {
this.modifier = 'meta';
} else if (event.shiftKey) {
this.modifier = 'shift';
}

// Clear the modifier if we don't use it
setTimeout(() => this.modifier = null, 200);
}

/** Check whether the toggle's model(s) are selected and set state. */
_checkSelected() {
Copy link
Member

Choose a reason for hiding this comment

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

The name _checkSelected doesn't communicate what this function is doing. The description could also be more specific

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about _updateSelected and Update the state of the selection based on the selection model.?

if (Array.isArray(this.model)) {
let has = true;
for (const model of this.model) {
if (!this._selection._selectionModel.isSelected(model)) {
has = false;
break;
}
}
this.selected = has;
} else {
this.selected = this._selection._selectionModel.isSelected(this.model);
}

this._cd.markForCheck();
}

}
59 changes: 59 additions & 0 deletions src/cdk-experimental/selection/selection.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
The `selection` package handles managing selection state on components
Copy link
Member

Choose a reason for hiding this comment

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

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.

with support for single and multi select with keyboard modifiers.

Components such as buttons, checkboxes, etc can all be decorated with
the `cdkSelectionToggle` directive. Once decorated, the directive
will listen for mouse and keyboard events and coordinate the
selection options with the parent `cdkSelection` directive.

In the example below, the div container of the buttons is decorated with `cdkSelection`. To populate
default selections pass an array of the selections to this option like:
`[cdkSelection]="['yellow']"`. Nested beneath the selection directive is a button decorated
with the `cdkSelectionToggle` directive with an argument of the item's value property.

```html
<ul cdkSelection cdkSelectionStrategy="single">
<li *ngFor="let item of items">
<button [cdkSelectionToggle]="item.value">
{{item.label}}
</button>
</li>
</ul>
```

### Strategies
The selection directive has 3 different strategies for selection:

- Single: Only one item can be selected at a time.
- Multiple: Multiple items can be selected by clicking
- Modifier Multiple: Multiple items can be select using keyboard modifiers such as shift or ctrl.

### Clearable
The ability to clear a selection that has been made can be prevented using the `cdkSelectionClearable`.
This means that after a selection (or many selections) have been made users can deselect
all but one of the selections. This concept can be related to radio buttons selection strategy.

### Tracking
When using complex objects with the selection toggle, custom tracking strategies
can ensure correct identification of the values. The `cdkSelectionTrackBy` accepts
a function that will be invoked with the values of the toggle. This can be used to
return a identifying attribute for the object.

```TS
@Component({
template: `
<ul cdkSelection cdkSelectionStrategy="multiple" [cdkSelectionTrackBy]=""trackBy>
<li *ngFor="let item of items">
<button [cdkSelectionToggle]="item">
{{item.label}}
</button>
</li>
</ul>
`
})
export class MyComponent {
trackBy(model) {
return model.value;
}
}
```
Loading