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

fix(select): unable to use the MatOption select/deselect API to toggle options #11528

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

crisbeto
Copy link
Member

Reworks the way MatSelect handles syncing its selected state between its child options and the SelectionModel, making it easier to follow and generally more robust. Previously we kept syncing the selected state in parallel between the options themselves and the selection model, which meant that we had to prevent infinite loops by ignoring any non-user changes to the options' selected state. This made the MatOption.select and MatOption.deselect methods useless to consumers and was very error prone. The new approach makes the select and deselect methods usable and allows us to turn the MatOption.selected property into an input, getting it closer to the native option API.

These changes also address the following issues that I bumped into along the way:

  • The MatOption.onSelectionChange was emitting even if the selection hadn't changed.
  • The SelectionModel.sort wasn't sorting its values if the consumer hadn't attempted to access the selected value since the last time it changed.

Fixes #9314.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label May 26, 2018
…e options

Reworks the way `MatSelect` handles syncing its selected state between its child options and the `SelectionModel`, making it easier to follow and generally more robust. Previously we kept syncing the selected state in parallel between the options themselves and the selection model, which meant that we had to prevent infinite loops by ignoring any non-user changes to the options' `selected` state. This made the `MatOption.select` and `MatOption.deselect` methods useless to consumers and was very error prone. The new approach makes the `select` and `deselect` methods usable and allows us to turn the `MatOption.selected` property into an input, getting it closer to the native `option` API.

These changes also address the following issues that I bumped into along the way:
* The `MatOption.onSelectionChange` was emitting even if the selection hadn't changed.
* The `SelectionModel.sort` wasn't sorting its values if the consumer hadn't attempted to access the `selected` value since the last time it changed.

Fixes angular#9314.
@crisbeto crisbeto force-pushed the 9314/select-selection-refactor branch from 202609b to 7343e40 Compare May 27, 2018 07:30
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

It wouldn't surprise me if some people were depending on the superfluous emits in tests, we'll see how the presubmit goes.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Jun 1, 2018
@jelbourn jelbourn merged commit 81537af into angular:master Jun 12, 2018
victoriaaa234 pushed a commit that referenced this pull request Jul 25, 2018
…e options (#11528)

Reworks the way `MatSelect` handles syncing its selected state between its child options and the `SelectionModel`, making it easier to follow and generally more robust. Previously we kept syncing the selected state in parallel between the options themselves and the selection model, which meant that we had to prevent infinite loops by ignoring any non-user changes to the options' `selected` state. This made the `MatOption.select` and `MatOption.deselect` methods useless to consumers and was very error prone. The new approach makes the `select` and `deselect` methods usable and allows us to turn the `MatOption.selected` property into an input, getting it closer to the native `option` API.

These changes also address the following issues that I bumped into along the way:
* The `MatOption.onSelectionChange` was emitting even if the selection hadn't changed.
* The `SelectionModel.sort` wasn't sorting its values if the consumer hadn't attempted to access the `selected` value since the last time it changed.

Fixes #9314.
@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.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unexpected behavior with mat-option select()
3 participants