Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(cdk-experimental/listbox): clean up the listbox API and make it work with forms #24920

Merged
merged 10 commits into from
May 26, 2022
Merged

fix(cdk-experimental/listbox): clean up the listbox API and make it work with forms #24920

merged 10 commits into from
May 26, 2022

Conversation

mmalerba
Copy link
Contributor

No description provided.

@mmalerba
Copy link
Contributor Author

Tests are a mess right now, but wanted to get some feeback on what I have so far. There's a couple questions I had while working on it which are called out in TODOs

mmalerba
mmalerba commented May 13, 2022
View reviewed changes
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
crisbeto
crisbeto reviewed May 18, 2022
View reviewed changes
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
jelbourn
jelbourn reviewed May 18, 2022
View reviewed changes
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba force-pushed the cdk-listbox branch 2 times, most recently from 164460a to 81d3d90 Compare May 20, 2022 00:11
@mmalerba
Copy link
Contributor Author

PTAL, made some changes and left some responses above

crisbeto
crisbeto reviewed May 20, 2022
View reviewed changes
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
src/cdk-experimental/listbox/listbox.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba marked this pull request as ready for review May 20, 2022 22:05
@mmalerba mmalerba requested review from a team and andrewseguin as code owners May 20, 2022 22:05
@mmalerba mmalerba added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label May 20, 2022
@github-actions
Copy link

github-actions bot commented May 20, 2022 •
edited
Loading

Deployed dev-app to: https://ng-comp-dev--pr-24920-a51e8ffa965e1319c806e8dc1b1fb8ba-w7fazh0v.web.app

@devversion devversion removed the request for review from a team May 23, 2022 13:23
@mmalerba
Copy link
Contributor Author

@jelbourn @crisbeto tests should hopefully be working now

@mmalerba mmalerba added the target: minor This PR is targeted for the next minor release label May 25, 2022
crisbeto
crisbeto approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@crisbeto crisbeto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit otherwise LGTM

src/cdk/collections/selection-model.ts Outdated Show resolved Hide resolved
@mmalerba mmalerba assigned mmalerba and unassigned jelbourn May 25, 2022
@mmalerba mmalerba added the action: merge The PR is ready for merge by the caretaker label May 25, 2022
@mmalerba mmalerba merged commit 0b59637 into angular:main May 26, 2022
@sync-by-unito sync-by-unito bot mentioned this pull request Jun 21, 2022
Closed
@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jun 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn left review comments

@crisbeto crisbeto crisbeto approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

Assignees

@mmalerba mmalerba

Labels
action: merge The PR is ready for merge by the caretaker dev-app preview When applied, previews of the dev-app are deployed to Firebase target: minor This PR is targeted for the next minor release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mmalerba @jelbourn @crisbeto