feat(chip-list): Implement FormFieldControl and ControlValueAccessor in chip list #6686

tinayuangao · 2017-08-29T03:33:22Z

Made md-chip's API similar to md-option
Made md-chip-list's API similar to md-select

Breaking changes in MdChip:

Removed @Output() select = EventEmitter<MdChipEvent> and @Output() deselect = EventEmitter<MdChipEvent>
Added @Output() onSelectionChange = EventEmitter<MdChipSelectionChange>

mmalerba

Still need to take a closer look at the code, but something I noticed in the demo: it's hard to focus the actual input sometimes. Clicking on the md-form-field / md-chip-list should focus the input rather than one of the chips

mmalerba · 2017-08-29T19:03:34Z

src/lib/chips/chip-input.ts

  }
 })
-export class MdChipInput {
+export class MdChipInput extends MdInput {


Should we check for something like: <input mdInput mdChipInputFor="chips"> and throw an error? People might just be used to adding the mdInput and I'm not sure what would happen in this case if they did that... I assume it would break things.

I think it's okay to use something like <input mdInput mdChipInputFor="chips">.

They won't conflict with each other since this extends MdInput? It seems like at the very least you would be doing twice the amount of work for no reason, since both directives would react to every change in value and try to do things.

Actually looking at the code a little more closely, I see that MdChipList implements MdFormFieldControl and handles communication with the MdFormField so why does this need to extend MdInput at all? If there's enough shared stuff between this and MdInput you could maybe factor it into a common base class, but I don't think it really makes sense for this to directly extend MdInput

@tinayuangao still curious about why this needs to extend MdInput, seems unnecessary to me.

@mmalerba Changed MdChipInput to not extends MdInput. Added focus(), focused and empty to work with chip list. Please take another look. Thanks!

jelbourn

Are there breaking changes that should be marked in this PR?

jelbourn · 2017-08-30T00:51:54Z

src/lib/chips/chip-input.ts

  value: string;
 }

 @Directive({


Could you add a class description for this directive that talks about how it operates as a control for md-form-field?

jelbourn · 2017-08-30T00:57:38Z

src/lib/chips/chip-list.ts

+  protected _ariaDescribedby: string;
+
+  /** Id of the chip list */
+  protected  _id: string;


There's an extra space after protected on some of these properties

jelbourn · 2017-08-30T00:59:15Z

src/lib/chips/chip-list.ts

+
+  /** For FormFieldControl. If any of the chips has focus, or the chip input has focus */
+  get focused() {
+    return !!this.chips.find((chip) => chip._hasFocus) ||


You can use some instead of find and you won't need the !!

jelbourn · 2017-08-30T00:59:57Z

src/lib/chips/chip-list.ts

+      return this._chipInput.placeholder;
+    } else {
+      return this._placeholder;
+    }


Use a ternary?

return this._chipInput ? this._chipInput.placeholder : this._placeholder;

jelbourn · 2017-08-30T01:01:05Z

src/lib/chips/chip-list.ts

  changeDetection: ChangeDetectionStrategy.OnPush
 })
-export class MdChipList implements AfterContentInit, OnDestroy {
+export class MdChipList implements MdFormFieldControl<any>, ControlValueAccessor,


Could you file an issue to track sharing common code for role="listbox" components in cdk/listbox? It would be good to capture what in particular would belong there

jelbourn · 2017-08-30T01:03:30Z

src/lib/chips/chip-list.ts

-          } else if (chipIndex - 1 >= 0) {
-            this._keyManager.setActiveItem(chipIndex - 1);
-          }
+  protected _updateKeyManager(chip: MdChip) {


Add function description?

jelbourn · 2017-08-30T01:04:49Z

src/lib/chips/chip-list.ts

+
+  _setSelectionByValue(value: any, isUserInput: boolean = true) {
+    this._clearSelection();
+    this.chips.forEach((chip) => chip.deselect());


Nit: don't need parenthesis around chip
(in any arrow function where the type is implicit)

jelbourn · 2017-08-30T01:05:34Z

src/lib/chips/chip-list.ts

+  _setSelectionByValue(value: any, isUserInput: boolean = true) {
+    this._clearSelection();
+    this.chips.forEach((chip) => chip.deselect());
+    const isArray = Array.isArray(value);


I wouldn't add the extra isArray variable here and would just use isArray() in the if block

jelbourn · 2017-08-30T01:05:47Z

src/lib/chips/chip-list.ts

+   */
+  private _selectValue(value: any, isUserInput: boolean = true): MdChip | undefined {
+
+    const correspondingChip = this.chips.find((chip: MdChip) => {


The MdChip type should be implicit

tinayuangao · 2017-08-30T23:25:51Z

Comments addressed. Added breaking changes to description.

For focus, user can set tabIndex to -1 to default focus on chip input.

<md-chip-list [tabIndex]="-1" #chipList> 
  <md-chip>chip one</md-chip>
  <input mdChipInputFor="chipList">
</md-chip-list>

And user can use left and right arrow keys to navigate to chips when tabIndex = -1.

kara

Typos and a few questions

kara · 2017-08-31T15:42:39Z

src/lib/chips/chip-input.ts


+/**
+ * A material design chips input component working with chip list component.
+ * To be placed inside md-form-field. Can be placed inside <md-chip-list> or outside <md-chip-list>.


Nit: Use < > with md-form-field, since we are using them with md-chip-list later in the comment

kara · 2017-08-31T15:43:41Z

src/lib/chips/chip.ts

  }
-  protected _selected: boolean = false;
+
+  /** The value of the chip. Default to the content inside <md-chip> tags. */


Typo: Default -> Defaults

kara · 2017-08-31T15:46:12Z

src/lib/chips/chip-list.ts

+import {MdFormFieldControl} from '../form-field/form-field-control';
+
+
+let nextUniqueId = 0;


Comment for this?

kara · 2017-08-31T15:46:59Z

src/lib/chips/chip-list.ts

+
+  protected _value: any;
+
+  /** Placeholder for the chip list. Alternatively, placeholder can set to MdChipInput */


Typo: can set to -> can be set on

kara · 2017-08-31T15:55:28Z

src/lib/chips/chips.scss

 }
+
+input.mat-chip-input {
+  width: 150px;


Should this be a Sass variable?

kara · 2017-08-31T16:01:26Z

src/lib/chips/chip-list.ts

+    if (this._isValidIndex(chipIndex)) {
+      if (chip._hasFocus) {
+        // Check whether the chip is the last item
+        if (chipIndex < this.chips.length - 1) {


Nit: we could clarify this comment. The check is really for whether the chip is not the last item, right? I had to read it a few times before I realized it was doing the opposite

kara · 2017-08-31T16:05:33Z

src/lib/chips/chip-list.ts

+    const isSubmitted = (this._parentFormGroup && this._parentFormGroup.submitted) ||
+      (this._parentForm && this._parentForm.submitted);
+
+    return !!(isInvalid && (isTouched || isSubmitted));


This looks pretty similar to the select's default error state here: https://github.com/angular/material2/blob/master/src/lib/select/select.ts#L678

Could we share it somewhere? There is a default errorStateMatcher - could we update it to add the control existence checks?

We have #6741 to track sharing common code.

Ah, good to know, thanks.

kara · 2017-08-31T16:06:34Z

src/lib/chips/chip-list.ts

+
+  /**
+   * Whether or not this chip is selectable. When a chip is not selectable,
+   * it's selected state is always ignored.


Typo: it's -> its

kara · 2017-08-31T16:07:05Z

src/lib/chips/chip-list.ts

+  @Output() change: EventEmitter<MdChipListChange> = new EventEmitter<MdChipListChange>();
+
+  /**
+   * Event that emits whenever the raw value of the select changes. This is here primarily


Typo: select -> chip list

kara · 2017-08-31T16:09:55Z

src/lib/chips/chip-list.ts

+      // A different comparator means the selection could change.
+      this._initializeSelection();
+    }
+  }


Could we share more code with the select here? Maybe a mixin that adds compareWith and default compareWith?

tinayuangao · 2017-08-31T17:44:03Z

Thanks for review. Comments addressed. Please take another look. Thanks!

jelbourn

@tinayuangao can you add the deprecated message to the commit message? That's what the changelog script picks up

Might be easiest to squash

Does chips.md need to be updated at all for this change?

jelbourn · 2017-08-31T18:52:37Z

src/lib/chips/chip-input.ts

+/**
+ * A material design chips input component working with chip list component.
+ * To be placed inside <md-form-field>. Can be placed inside <md-chip-list> or outside
+ * <md-chip-list>.


How about

Directive that adds chip-specific behaviors to an input element inside <md-form-field>. May be placed inside or outside of an <md-chip-list>.

jelbourn · 2017-08-31T18:53:20Z

src/lib/chips/chip-input.ts

+ *         <md-chip>Chip 2<md-chip>
+ *         <input mdChipInputFor="chipList">
+ *       </md-chip-list>
+ *     </md-form-field>


I think it's better to leave the examples in the docs / live examples, since the generated API don't wont give this a nice visual treatment

jelbourn · 2017-08-31T18:57:50Z

src/lib/chips/chip-list.ts

+    '[attr.aria-describedby]': '_ariaDescribedby || null',
+    '[attr.aria-required]': 'required.toString()',
+    '[attr.aria-disabled]': 'disabled.toString()',
+    '[attr.aria-invalid]': 'errorState',


Does the component support multiple selection? If not, it should have aria-multiselectable="false"

Now it only support multiple selection.

I think I should change it to support single selection so we can share more common code with select component.

Added single selection support to chip list. Please take another look. Thanks!

jelbourn · 2017-08-31T18:59:19Z

src/lib/chips/chip-list.ts

+  /** Comparison function to specify which option is displayed. Defaults to object equality. */
+  private _compareWith = (o1: any, o2: any) => o1 === o2;
+
+  get selected(): MdChip[] {


Add description?

jelbourn · 2017-08-31T19:00:40Z

src/lib/chips/chip-list.ts

+    return this._chipInput ? this._chipInput.placeholder : this._placeholder;
+  }
+
+  /** For FormFieldControl. If any of the chips has focus, or the chip input has focus */


/** Whether any chips or the mdChipInput inside of this chip-list has focus. */

jelbourn · 2017-08-31T19:02:21Z

src/lib/chips/chip-list.ts

+    return (!this._chipInput || this._chipInput.empty) && this.chips.length === 0;
+  }
+
+  /** For FormFieldControl. The disabled is not depend on chip input */


/** Whether this chip-list is disabled. */

jelbourn · 2017-08-31T19:02:38Z

src/lib/chips/chip-list.ts

+  get disabled() { return this.ngControl ? this.ngControl.disabled : this._disabled; }
+  set disabled(value: any) { this._disabled = coerceBooleanProperty(value); }
+
+  get errorState(): boolean {


Add description?

jelbourn · 2017-08-31T19:24:58Z

src/lib/chips/chip-list.ts

+    }
+  }
+
+  _onBlur() {


It's weird that there's a _blur() and an _onBlur()- could the names be more specific? Function descriptions would also help

jelbourn · 2017-08-31T19:25:48Z

src/lib/chips/chip-list.ts

+    if (!this.disabled) {
+      if (this._chipInput) {
+        // Check if focus moved to chip input. If not, do real on blur
+        setTimeout(() => {


Add comment for why this needs to be a timeout?

jelbourn · 2017-08-31T19:26:24Z

src/lib/chips/chip-list.ts

+  }
+
+  _blur() {
+    if (!this.disabled) {


Would be good to have a comment that explains what this method is doing

tinayuangao · 2017-09-01T00:17:09Z

Comments addressed. Updated chips.md and commit message. Please take another look. Thanks!

jelbourn · 2017-09-01T21:51:02Z

@tinayuangao prerender test looks to be failing now

tinayuangao · 2017-09-07T00:32:32Z

@jelbourn Fixed prerender test. Please take a look. Thanks!

jelbourn

LGTM, just needs rebase

tinayuangao · 2017-09-14T01:01:35Z

Fixed focus & blur behavior for chip list. PTAL. Thanks!

mmalerba

LGTM

jelbourn

LGTM

@output

BREAKING CHANGES: - Deprecated: @output() select, and @output() deselect - Added @output() onSelectionChange = EventEmitter<MdChipSelectionChange>

angular-automatic-lock-bot · 2019-09-07T04:02:58Z

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

tinayuangao requested a review from mmalerba as a code owner August 29, 2017 03:33

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Aug 29, 2017

tinayuangao requested review from jelbourn and kara August 29, 2017 03:33

tinayuangao added the pr: needs review label Aug 29, 2017

tinayuangao assigned kara, jelbourn and mmalerba Aug 29, 2017

mmalerba reviewed Aug 30, 2017

View reviewed changes

tinayuangao force-pushed the chip-input branch from 8ae9963 to eb74659 Compare August 30, 2017 00:40

jelbourn reviewed Aug 30, 2017

View reviewed changes

tinayuangao force-pushed the chip-input branch from eb74659 to c19c278 Compare August 30, 2017 23:22

kara reviewed Aug 31, 2017

View reviewed changes

kara removed their assignment Aug 31, 2017

jelbourn reviewed Aug 31, 2017

View reviewed changes

tinayuangao force-pushed the chip-input branch 2 times, most recently from dd2f0a0 to 234f0d2 Compare September 1, 2017 00:15

tinayuangao force-pushed the chip-input branch 2 times, most recently from 0cccb44 to 96ca320 Compare September 1, 2017 17:09

jelbourn approved these changes Sep 13, 2017

View reviewed changes

jelbourn added pr: lgtm and removed pr: needs review labels Sep 13, 2017

tinayuangao force-pushed the chip-input branch from 8117d07 to 19f0842 Compare September 13, 2017 17:16

tinayuangao removed the pr: needs rebase label Sep 14, 2017

mmalerba approved these changes Sep 14, 2017

View reviewed changes

tinayuangao added the P1 Impacts a large percentage of users; if a workaround exists it is partial or overly painful label Sep 14, 2017

jelbourn approved these changes Sep 14, 2017

View reviewed changes

jelbourn added the action: merge The PR is ready for merge by the caretaker label Sep 14, 2017

tinayuangao added 4 commits September 15, 2017 10:11

feat(chip-list): Implement FormFieldControl

d262ff7

BREAKING CHANGES: - Deprecated: @output() select, and @output() deselect - Added @output() onSelectionChange = EventEmitter<MdChipSelectionChange>

Add multiple selection and single selection to chip list

ae03bfe

try fix test

fc5f6e9

not extend from MdInput

15106aa

tinayuangao force-pushed the chip-input branch from 0416022 to 0d75c2a Compare September 15, 2017 17:13

Fix blur & focus behavior

1bfa13e

tinayuangao force-pushed the chip-input branch from 0d75c2a to 1bfa13e Compare September 15, 2017 17:18

tinayuangao added 2 commits September 15, 2017 10:35

.

9cb4713

Merge branch 'master' into chip-input

93ad446

jelbourn merged commit 7a42706 into angular:master Sep 18, 2017

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

		import {MdFormFieldControl} from '../form-field/form-field-control';


		let nextUniqueId = 0;


		protected _value: any;

		/** Placeholder for the chip list. Alternatively, placeholder can set to MdChipInput */

feat(chip-list): Implement FormFieldControl and ControlValueAccessor in chip list #6686

feat(chip-list): Implement FormFieldControl and ControlValueAccessor in chip list #6686

Uh oh!

Conversation

tinayuangao commented Aug 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmalerba 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!

mmalerba Aug 31, 2017 • 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!

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!

tinayuangao commented Aug 30, 2017

Uh oh!

kara 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!

Choose a reason for hiding this comment

Uh oh!

tinayuangao commented Aug 31, 2017

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

tinayuangao commented Aug 29, 2017 •

edited

Loading

mmalerba Aug 31, 2017 •

edited

Loading

jelbourn Aug 31, 2017 •

edited

Loading