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(api): updates the api to standard naming and adds docs for selection #8286

Merged
merged 3 commits into from
Aug 28, 2018
Merged

fix(api): updates the api to standard naming and adds docs for selection #8286

merged 3 commits into from
Aug 28, 2018

Conversation

amcdnl
Copy link
Contributor

@amcdnl amcdnl commented Nov 7, 2017

This PR updates the change event in SelectionModel from onChange to changed to match the new api standard. It also adds basic docs for the selection model utility.

@amcdnl amcdnl self-assigned this Nov 7, 2017
@amcdnl amcdnl requested a review from jelbourn November 7, 2017 22:51
@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 7, 2017
jelbourn
jelbourn reviewed Nov 8, 2017
View reviewed changes
src/cdk/collections/collections.md Outdated
### `SelectionModel`
`SelectionModel` is a utility for powering selection of one or more options from a list.
This utility is used in components such as the selection list, table selections and chip lists.
Copy link
Member

Choose a reason for hiding this comment

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

Something like

`SelectionModel` stores the selection state for components that support multiple selection.

The word "utility" isn't a good fit here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about model?

Copy link
Member

Choose a reason for hiding this comment

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

That works

src/cdk/collections/selection.ts
@@ -34,7 +34,13 @@ export class SelectionModel<T> {
}

/** Event emitted when the value has changed. */
onChange: Subject<SelectionChange<T>> | null = this._emitChanges ? new Subject() : null;
changed: Subject<SelectionChange<T>> | null = this._emitChanges ? new Subject() : null;
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we land on change present-tense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We landed on myEventThingChange but the rest of them ended up past tense.

Copy link
Member

Choose a reason for hiding this comment

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

Which things are changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A few I know of selectedChanged, _scrollDistanceChanged and _selectedIndexChanged. Let me know what you want to do.

@josephperrott josephperrott added pr: needs rebase target: patch This PR is targeted for the next patch release labels Jun 26, 2018
@ngbot
Copy link

ngbot bot commented Jul 11, 2018

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

1 similar comment
@ngbot
Copy link

ngbot bot commented Jul 11, 2018

Hi @amcdnl! This PR has merge conflicts due to recent upstream merges.
Please help to unblock it by resolving these conflicts. Thanks!

@amcdnl
Copy link
Contributor Author

amcdnl commented Jul 15, 2018

bump @jelbourn

jelbourn
jelbourn reviewed Jul 16, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

One last comment

src/cdk/collections/selection.ts

/**
* Event emitted when the value has changed.
* @deprecated Use `changed` instead.
Copy link
Member

Choose a reason for hiding this comment

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

Need to add a deletion-target here (8.0)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

@amcdnl amcdnl mentioned this pull request Jul 22, 2018
Closed
jelbourn
jelbourn approved these changes Jul 24, 2018
View reviewed changes
Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

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

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed and removed pr: needs rebase labels Jul 24, 2018
@jelbourn
Copy link
Member

Commit message fix-up: should use feat(collections) and also mention the API deprecation

@jelbourn
Copy link
Member

@amcdnl github shows this PR as having no conflicts, but our presubmit script thinks it does. Could you rebase it?

@jelbourn jelbourn merged commit 27e88c3 into angular:master Aug 28, 2018
jelbourn pushed a commit that referenced this pull request Aug 29, 2018
@amcdnl @jelbourn
fix(collections): align SelectionModel to changed naming (#8286)
4b30539
devversion added a commit to devversion/material2 that referenced this pull request Sep 13, 2018
@devversion
refactor(schematics): constructor check rule does not check all diagn…
04edb81 
…ostics

* Currently if the node of a diagnostic couldn't be resolved, or the `className` is not part of the upgrade data, we accidentally `return` instead of `continuing` the `for of` loop that checks every determined diagnostic.
* Adds missing upgrade data for a V7 merged breaking change: angular#8286
* Renames the `test-cases/v5` directory to `test-cases/v6` because the test-cases are running against version target: `v6`.
* Adds test cases for V7 constructor checks data; test cases for property name change from angular#8286
@devversion devversion mentioned this pull request Sep 13, 2018
Merged
jelbourn pushed a commit that referenced this pull request Sep 13, 2018
@devversion @jelbourn
refactor(schematics): constructor check rule does not check all diagn…
90af397 
…ostics (#13103)

* Currently if the node of a diagnostic couldn't be resolved, or the `className` is not part of the upgrade data, we accidentally `return` instead of `continuing` the `for of` loop that checks every determined diagnostic.
* Adds missing upgrade data for a V7 merged breaking change: #8286
* Renames the `test-cases/v5` directory to `test-cases/v6` because the test-cases are running against version target: `v6`.
* Adds test cases for V7 constructor checks data; test cases for property name change from #8286
@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 Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jelbourn jelbourn jelbourn approved these changes

@andrewseguin andrewseguin Awaiting requested review from andrewseguin

@crisbeto crisbeto Awaiting requested review from crisbeto

Assignees

@amcdnl amcdnl

Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement merge: fix commit message When the PR is merged, rewrites/fixups of the commit messages are needed target: patch This PR is targeted for the next patch release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@amcdnl @jelbourn @googlebot @josephperrott