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.

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

feat(selection-model): de/select multiple values at the same time #7001

Merged

Conversation

devversion
Copy link
Member

@devversion devversion commented Sep 12, 2017

Adds support for passing multiple values to the SelectionModel at the same time. This feature is described in the JSDoc already, but just wasn't implemented yet.

This functionality gives us more control about the onChange event, which will otherwise fire every time if for example multiple values need to be selected.

Related to #6896

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Sep 12, 2017
@@ -162,6 +164,13 @@ export class SelectionModel<T> {
this._selection.forEach(value => this._unmarkSelected(value));
}
}

/** Throws an error if multiple values are passed into a selection model with a single value. */
private _warnMultipleValuesForSingleSelection(values: T[]) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think the name is too accurate since it'll throw an error, not warn.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.


/** Throws an error if multiple values are passed into a selection model with a single value. */
export function throwMultipleValuesInSingleSelectionError() {
throw Error('Cannot pass multiple values into SelectionModel with single-value mode.');
Copy link
Member

@crisbeto crisbeto Sep 12, 2017

Choose a reason for hiding this comment

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

Since it is only being used in one place, consider moving this it into _warnMultipleValuesForSingleSelection?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we have a convention that we create functions for these errors and re-export the Error object.

e.g in https://github.com/angular/material2/blob/master/src/lib/select/select-errors.ts#L28

Updating to follow that convention

Copy link
Member

Choose a reason for hiding this comment

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

Those errors are in a function so we can use them in tests. If it's a one-off, it's fine to have it inline.

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.

LGTM, one nit. Add label when ready.

@@ -182,6 +185,6 @@ export class SelectionChange<T> {
}

/** Throws an error if multiple values are passed into a selection model with a single value. */
Copy link
Member

Choose a reason for hiding this comment

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

Comment is inaccurate now. It returns an error instead of throwing it.

@devversion devversion force-pushed the feat/selection-model-multiple-values branch from 6615c9a to b629c15 Compare September 12, 2017 11:40
@devversion devversion added the action: merge The PR is ready for merge by the caretaker label Sep 12, 2017
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

@devversion devversion added the P2 The issue is important to a large percentage of users, with a workaround label Sep 18, 2017
Adds support for passing multiple values to the SelectionModel at the same time. This feature is described in the JSDoc already, but just wasn't implemented yet.

This functionality gives us more control about the `onChange` event, which will otherwise fire every time if for example multiple values need to be selected.
@devversion devversion force-pushed the feat/selection-model-multiple-values branch from b629c15 to 980ce23 Compare September 21, 2017 15:31
@andrewseguin andrewseguin merged commit e52beeb into angular:master Sep 29, 2017
@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 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement P2 The issue is important to a large percentage of users, with a workaround
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants