Add chips keyboard support. #2046

topherfangio · 2016-12-01T17:55:11Z

Add basic focus/keyboard support for chips.

Up/down arrows navigate chips.
Clicking a chip properly focuses it for subsequent keyboard
navigation.
More demos.
Linter passes
Tests pass
Confirmed AoT compatibility

References #120.

Demo Page Screen Shot

crisbeto · 2016-12-01T18:04:41Z

FYI the ListKeyManager fix was already merged in with #2009.

jelbourn · 2016-12-01T19:15:16Z

Looks like you have one failure on IE11

topherfangio · 2016-12-01T20:06:46Z

@jelbourn I'll look into the failure tonight. In the meantime, can you do a code review so I can make any additional changes you find?

jelbourn · 2016-12-01T19:16:57Z

src/lib/chips/_chips-theme.scss

  .md-chip {
    background-color: #e0e0e0;
    color: rgba(0, 0, 0, 0.87);
  }

  .md-chip.selected {
+    // TODO: Based on spec, this should be #808080, but we can only use md-contrast with a palette
+    background-color: md-color($md-grey, 600);
+    color: md-contrast($md-grey, 600);


It's fine to use a different color so long as it's handled for both light and dark theme.
(given it's not affected by primary/accent)

I can use a custom background color, but is there a good way to do the md-contrast() with a standard color instead of the palette color? If not, I guess I can manually calculate it based on the spec.

The contrast colors aren't calculated, it's just whatever a human designer thought looked "right". Where in the spec does it say #808080?

I guess it's never "stated" in the spec; I just used a color picker and pulled it from the spec images (which all appear to be consistent).

jelbourn · 2016-12-01T19:31:32Z

src/lib/chips/chip-list.ts

+ *
+ * @type Component
+ */
+export const MD_CHIP_LIST_COMPONENT_CONFIG: Component = {


Kara and I chatted about this for a bit and decided that we'd rather duplicate the component metadata for now rather than breaking it out like this. Couple of reasons for this:

It adds indirection to where the actual setup for the component lives

The host object is effectively static; you'd have to duplicate the whole thing anyway if you wanted to add a property. With inputs and outputs, the metadata extractor does support concatenation, but there's no equivalent for objects yet (though that is in the pipeline).

Doing outputs: ['focus: onFocus'] doesn't seem to work (it should work like @Output('focus') onFocus, possibly a bug in core).

jelbourn · 2016-12-01T19:33:33Z

src/lib/chips/chip-list.ts

+
+  /**
+   * The chip components contained within this chip list.
+   */


You can smush short description like this like:

/** The chip components contained within this chip list. */

jelbourn · 2016-12-01T19:34:43Z

src/lib/chips/chip-list.ts

+
+  /********************
+   * EVENTS
+   ********************/


I consider section delimiting comments like this to be a code smell. In general, if the file is big enough to need them, you should refactor. (this file isn't big enough to need them)

jelbourn · 2016-12-01T19:37:06Z

src/lib/chips/chip.ts

+
+  /**
+   * Emitted when the chip receives focus.
+   * @type {EventEmitter}


Omit all types from JsDoc. These are captured by TypeScript.

jelbourn · 2016-12-01T20:00:03Z

src/lib/chips/chip.ts

+   * Emitted when the chip is destroyed.
+   * @type {EventEmitter}
+   */
+  public destroy = new EventEmitter();


No need to include the public keyword; we always omit it since it's the default

jelbourn · 2016-12-01T20:00:49Z

src/lib/chips/chip.ts

+   * Emitted when the chip receives focus.
+   * @type {EventEmitter}
+   */
+  public didfocus = new EventEmitter();


EventEmitter needs a generic type. If you're never passing any data to it, use void. If you're passing data, create a type to serve as the event object.

jelbourn · 2016-12-01T20:02:24Z

src/lib/chips/chip.ts

+   *
+   * @returns {String}
+   */
+  get isAriaDisabled(): string {


This should be internal (start with an underscore but not use the private keyword)

jelbourn · 2016-12-01T20:04:22Z

src/lib/chips/chip.ts

+   * @param value `true` to disable, `false` to enable.
+   */
+  set disabled(value: boolean) {
+    this._disabled = (value === false || value === undefined) ? null : true;


Use coerceBooleanProperty

Don't need @param JsDoc for setters

jelbourn · 2016-12-01T20:10:40Z

src/lib/chips/chip.ts

+   *
+   * @param event
+   */
+  click(event: Event) {


This should be something like

_handleClick() { if (!this.disabled) { this.focus(); } } focus() { this._renderer.invokeElementMethod(this._elementRef.nativeElement, 'focus'); }

You shouldn't need the focus Output. The native focus event on the chip element should work fine.
http://plnkr.co/edit/wW5Ux0yc3JlgTZZATGrQ?p=preview

jelbourn

FYI, probably obsolete comments

jelbourn · 2016-12-05T21:08:47Z

src/lib/chips/chip-list-key-manager.ts

+ * keyboard navigation are properly handled.
+ */
+export class ChipListKeyManager extends ListKeyManager {
+  private _subscribed: MdBasicChip[] = [];


Needs field description

I also think a WeakMap would be better for what you're doing (WeakSet would be best, but that's not supported in IE11).

jelbourn · 2016-12-12T21:24:24Z

src/lib/chips/chip-list.ts

+  /** Track which chips we're listening to for focus/destruction. */
+  private _subscribed: WeakMap<MdChip, Boolean> = new WeakMap();
+
+  /** INTERNAL - The ListKeyManager which handles focus. */


Don't need INTERNAL- it's implied by the underscore

jelbourn · 2016-12-12T21:24:35Z

src/lib/chips/chip.spec.ts

@@ -1,47 +1,120 @@
+// http://plnkr.co/edit/6Nnren92D7C0zvRf0mwY?p=preview


Omit this plunker link

jelbourn · 2016-12-12T21:25:07Z

src/lib/chips/chip.ts

+  /**
+   * Emitted when the chip is destroyed.
+   */
+  @Output('destroy') destroy = new EventEmitter<MdChipEvent>();


Don't need to give a string name if it has the same name as the property

jelbourn · 2016-12-12T21:26:28Z

src/lib/chips/chip.ts

+  ngOnInit(): void {
+    let el: HTMLElement = this._elementRef.nativeElement;
+
+    if (el.tagName == 'MD-CHIP' || el.hasAttribute('md-chip')) {


Prefer nodeName to tagName and use toLowerCase()

jelbourn · 2016-12-12T21:29:18Z

src/lib/core/a11y/list-key-manager.ts

+   * @param index The index of the item to be focused.
+   * @param focusElement Whether or not to focus the element as well. Defaults to `true`.
+   */
+  setFocus(index: number, focusElement = true): void {


Can we break this into two methods? I think it would be better to have focusItem(index) and updateFocusedIndex(index) where the former sets focus and the latter only updates the index.

Should this just be set focusedItemIndex()?

I REALLY don't like doing manager.focusedItemIndex = ..., so I would prefer your suggestion of updateFocusedItemIndex(index), but I was putting it directly below the getter and it felt a bit weird.

Thoughts?

Actually, focus should be a no-op if the item is already focused. Why not just let it run again?

I'm not sure it is a no-op. I think it was emitting the events twice.

Also, now that I dug into the code, I'm not sure we want to change this. The majority of places assume it does both; my chips is a special case where we don't want to focus it (because it happens on click; so it already has focus).

Separating this forces users to call two functions the majority of the time which feels odd.

Calling this in order to perform the focus should still update the index, I was just proposing we add a new method that only updates the index.

topherfangio · 2016-12-13T17:02:32Z

@jelbourn Requested updates have been made. Let me know if you see anything else!

jelbourn

LGTM, just a last few style fixes I missed last time. Add the merge_ready label when you're done.

jelbourn · 2016-12-13T17:13:17Z

src/lib/chips/chip-list.ts

+export class MdChipList implements AfterContentInit {
+
+  /** Track which chips we're listening to for focus/destruction. */
+  private _subscribed: WeakMap<MdChip, Boolean> = new WeakMap();


boolean lower case

jelbourn · 2016-12-13T17:13:35Z

src/lib/chips/_chips-theme.scss

@@ -13,6 +13,11 @@
  }

  .md-chip.selected {
+    background-color: #808080;


Add comment explaining that this is the same color in both light and dark themes.

I can definitely do that; but do we have any specs for a dark theme? I think it would be nice to change it as the current chips in dark mode do look a bit out of place.

jelbourn · 2016-12-13T17:16:01Z

src/lib/chips/chip-list.ts

+  /** Pass relevant key presses to our key manager. */
+  keydown(event: KeyboardEvent) {
+    this._keyManager.onKeydown(event);
+  }


You don't actually need these two events: you can call _keyManager.focusFirstItem() and _keyManager.onKeydown($event) directly in the host binding.

I'll change the focus, but I do need the onKeydown for the selection/deletion support that is coming soon in the next PR.

jelbourn · 2016-12-13T17:16:38Z

src/lib/chips/chip-list.ts

+   * @param chips The list of chips to be subscribed.
+   */
+  protected subscribeChips(chips: QueryList<MdChip>): void {
+    chips.forEach((chip: MdChip) => {


You shouldn't need the type here, it should be inferred. Also can be condensed to

chips.forEach(chip => this.addChip(chip));

jelbourn · 2016-12-13T17:18:43Z

src/lib/chips/chip.ts

@@ -1,17 +1,96 @@
-import { Component, ElementRef, Renderer } from '@angular/core';
+import {
+  // Classes


Don't need comments like this in imports.

jelbourn · 2016-12-13T17:20:15Z

src/lib/chips/chip-list.spec.ts

+import { async, ComponentFixture, TestBed } from '@angular/core/testing';
+import { Component, DebugElement, QueryList } from '@angular/core';
+import { By } from '@angular/platform-browser';
+import { MdChip, MdChipList, MdChipsModule } from './index';


No spaces inside { } like this; there's a WebStorm setting for this.

Add basic focus/keyboard support for chips. - Up/down arrows navigate chips. - Clicking a chip properly focuses it for subsequent keyboard navigation. - More demos. Confirmed AoT compatibility. References angular#120.

angular-automatic-lock-bot · 2019-09-06T05:02:13Z

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.}

topherfangio added the pr: needs review label Dec 1, 2016

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 1, 2016

jelbourn reviewed Dec 1, 2016

View reviewed changes

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch 2 times, most recently from 1d96636 to 9183484 Compare December 1, 2016 22:17

jelbourn reviewed Dec 5, 2016

View reviewed changes

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch 3 times, most recently from 4041670 to b551fa8 Compare December 9, 2016 17:51

jelbourn reviewed Dec 12, 2016

View reviewed changes

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch from b551fa8 to 7ee819c Compare December 13, 2016 16:30

jelbourn reviewed Dec 13, 2016

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Dec 13, 2016

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch 2 times, most recently from 6ed7552 to 3a58643 Compare December 13, 2016 18:17

topherfangio added the action: merge The PR is ready for merge by the caretaker label Dec 13, 2016

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch from 3a58643 to d0c0d65 Compare December 13, 2016 20:34

Add chips keyboard support.

b138e66

Add basic focus/keyboard support for chips. - Up/down arrows navigate chips. - Clicking a chip properly focuses it for subsequent keyboard navigation. - More demos. Confirmed AoT compatibility. References angular#120.

topherfangio force-pushed the team/topher/chips-keyboard-focus-120 branch from d0c0d65 to b138e66 Compare December 13, 2016 23:03

jelbourn merged commit ba85883 into angular:master Dec 14, 2016

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019

		@@ -1,47 +1,120 @@
		// http://plnkr.co/edit/6Nnren92D7C0zvRf0mwY?p=preview

Add chips keyboard support. #2046

Add chips keyboard support. #2046

Conversation

topherfangio commented Dec 1, 2016 • edited Loading

Demo Page Screen Shot

crisbeto commented Dec 1, 2016

jelbourn commented Dec 1, 2016

topherfangio commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

topherfangio commented Dec 13, 2016

jelbourn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Sep 6, 2019

topherfangio commented Dec 1, 2016 •

edited

Loading