feat(list-key-manager): accept item references in setActiveItem #10029
Conversation
crisbeto
requested review from
devversion,
jelbourn,
kara and
tinayuangao
as code owners
googlebot
added
the
cla: yes
crisbeto
force-pushed
the
list-key-manager-set-item
branch
2 times, most recently
from
01f08ef to
8234653
Compare
| * previously active item. | ||
| * @param item Item to be set as active. | ||
| */ | ||
| setActiveItem(item: T): void; |
There was a problem hiding this comment.
Technically T could be number. It would avoid the ambiguity if we just kept the separate methods (setActiveItem and setActiveItemIndex)
There was a problem hiding this comment.
I don't think so. The _items that are passed in through the constructor are a QueryList which can't contain numbers.
There was a problem hiding this comment.
Is there ever a time when a component would want to use one or the other in a single call? Still seems like it would be more explicit to have both methods with index vs. item behaviors.
There was a problem hiding this comment.
It would be more explicit, but it adds a couple of extra methods to the API that do more or less the same. I don't think there's a case where a component would want to use both of them in the same call.
There was a problem hiding this comment.
Ack, I don't feel that strongly. My only other comment would be that the implemention method should be item: T|number
There was a problem hiding this comment.
That's what I was going for initially, but then TS seems to have a hard time figuring out the type when we pass the item to updateActiveItem. There were also some issues when trying to assign the active item inside the derived classes. I went with any since the implementation method signature won't show up in the type info anyway.
* Adds an overload to `setActiveItem` to allow for an item to be set as active, rather than its index. This avoids awkward conversions in certain cases (see the chips and select changes). * Deprecates the `updateActiveItemIndex` method in favor of `updateActiveItem` which accepts both an index and an item. * Fixes the active item not being updated when it is set via `updateActiveItemIndex`.
crisbeto
force-pushed
the
list-key-manager-set-item
branch
from
8234653 to
dfa2b7a
Compare
jelbourn
added
pr: lgtm
action: merge
|
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. |
setActiveItemto allow for an item to be set as active, rather than its index. This avoids awkward conversions in certain cases (see the chips and select changes).updateActiveItemIndexmethod in favor ofupdateActiveItemwhich accepts both an index and an item.updateActiveItemIndex.