Skip to content

ListKeyManager: change emission on QueryList changes #20978

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

Open
br-kwon opened this issue Nov 5, 2020 · 4 comments
Open

ListKeyManager: change emission on QueryList changes #20978

br-kwon opened this issue Nov 5, 2020 · 4 comments
Labels
area: cdk/a11y needs: discussion Further discussion with the team is needed before proceeding P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent

Comments

@br-kwon
Copy link

br-kwon commented Nov 5, 2020

this._activeItemIndex = newIndex;

In this if block, should this.changes also emit since it seems to emit indices.

@br-kwon br-kwon changed the title ListKeyManager emission on QueryList changes ListKeyManager: change emission on QueryList changes Nov 5, 2020
@crisbeto
Copy link
Member

crisbeto commented Nov 9, 2020

I'm not sure whether it should emit, because the active item is technically the same, it's just its index that has changed. There's a somewhat-related fix that will emit the event if the active item is removed #20008.

@br-kwon
Copy link
Author

br-kwon commented Nov 9, 2020

Yeah not sure either, but then I feel the changes property is quite ambiguous since it emits a number. I would expect something named changes emits when the state of the instance has changed (activeItemIndex in this case).

One challenge I faced is when I used this class in an unconventional manner. Instead of passing in QueryChildren grabbed from ViewChildren or ContentChildren, I passed in a new QueryList(). The intention here was to listen for ListKeyManager.changes() and markForCheck(). When I reset the list and notifyOnChanges, I would expect an already active item to be "active", but fails to do so in this case.

@crisbeto
Copy link
Member

That's totally valid, but I suspect that it may cause some breakages. E.g. in mat-select we have an event tied to the list manager event which will begin firing if items are added to the select automatically, if we were to make the proposed change. We could try it out once #20008 lands and see how many tests break.

@crisbeto crisbeto added area: cdk/a11y needs: discussion Further discussion with the team is needed before proceeding labels Nov 10, 2020
@br-kwon
Copy link
Author

br-kwon commented Nov 10, 2020

sounds great! Would love to take a pass at it

@andrewseguin andrewseguin added the P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent label Jun 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: cdk/a11y needs: discussion Further discussion with the team is needed before proceeding P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent
Projects
None yet
Development

No branches or pull requests

3 participants