fix(chips): Fix chip and chip list selectable #9955
Conversation
googlebot
added
the
cla: yes
src/lib/chips/chip-list.spec.ts
Outdated
| testComponent.selectable = false; | ||
| fixture.detectChanges(); | ||
|
|
||
| chips.forEach(chip => { |
There was a problem hiding this comment.
This one will end up generating a bunch of failure messages if we ever end up breaking the functionality. Something like this might be cleaner:
const chipsValid = chips.every(chip => !chip.selectable && !chip._elementRef.nativeElement.hasAttribute('aria-selected')):
expect(chipsValid).toBe(true);
src/lib/chips/chip-list.ts
Outdated
| @@ -274,12 +274,17 @@ export class MatChipList extends _MatChipListMixinBase implements MatFormFieldCo | |||
| @Input('aria-orientation') ariaOrientation: 'horizontal' | 'vertical' = 'horizontal'; | |||
|
|
|||
| /** | |||
| * Whether or not this chip is selectable. When a chip is not selectable, | |||
| * Whether or not this chips are selectable. When a chip is not selectable, | |||
There was a problem hiding this comment.
I think the comment still refers to an individual chip, but this is in the chip list.
src/lib/chips/chip-list.ts
Outdated
| set selectable(value: boolean) { | ||
| this._selectable = coerceBooleanProperty(value); | ||
| if (this.chips) { | ||
| this.chips.toArray().forEach(chip => chip.chipListSelectable = this._selectable); |
There was a problem hiding this comment.
IIRC the QueryList already has a forEach method so you shouldn't need the toArray call.
| */ | ||
| @Input() | ||
| get selectable(): boolean { return this._selectable; } | ||
| get selectable(): boolean { return this._selectable && this.chipListSelectable; } |
There was a problem hiding this comment.
Does this work with the OnPush change detection? I think the chip may need a markForCheck somewhere.
There was a problem hiding this comment.
The selectable on chip list is an @Input().
tinayuangao
force-pushed
the
fix-chip-selectable
branch
from
1e45139 to
e69e277
Compare
|
Thanks for review! |
crisbeto
added
pr: lgtm
action: merge
andrewseguin
added
the
target: minor
|
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. |
Fixes #9924