refactor(button-toggle): switch to SelectionModel #2773

crisbeto · 2017-01-23T21:41:35Z

Switches the MdButtonToggleGroup to use the SelectionModel.
Avoids having to define two toggle group directives.
No longer fires change events from actions that weren't generated by the user.

* Switches the MdButtonToggleGroup to use the SelectionModel. * Avoids having to define two toggle group directives. * No longer fires change events from actions that weren't generated by the user.

EladBezalel · 2017-02-15T22:07:32Z

@crisbeto can you please resolve the conflicts?
#2694 pr is based on some changes here

# Conflicts: # src/lib/button-toggle/button-toggle.html # src/lib/button-toggle/button-toggle.ts

crisbeto · 2017-02-15T22:22:19Z

Done @EladBezalel

EladBezalel · 2017-02-15T22:27:48Z

src/lib/button-toggle/button-toggle.ts

+
+  ngAfterContentInit(): void {
+    this._model = new SelectionModel<MdButtonToggle>(this.multiple,
+        this._tempValue ? [].concat(this._tempValue) : null);


you can do [this._tempValue] instead of [].concat(this._tempValue)

This will end up being wrong if the _tempValue is an array.

EladBezalel · 2017-02-15T22:28:57Z

src/lib/button-toggle/button-toggle.ts

-    this._isInitialized = true;
+    // Listen to changes in child toggles and re-subscribe when toggles are adde/removed.
+    this._subscribeToToggleChanges();
+    this._toggleSubscription = this._toggles.changes.subscribe(() => {


looks better IMO

this._toggleSubscription = this._toggles.changes.subscribe(() => this._subscribeToToggleChanges());

It exceeds the maximum line length from the linter config.

EladBezalel · 2017-02-15T22:34:46Z

src/lib/button-toggle/button-toggle.ts

-    this.value = selected ? selected.value : null;
+  /** Subscribes to user-generated changes of the button toggles. */
+  private _subscribeToToggleChanges(): void {
+    let source = Observable.merge.apply(Observable, this._toggles.map(toggle => toggle.change));


maybe mergeMap is a better solution?

I haven't really used mergeMap before, but it doesn't look like it's appropriate here. We only want to merge all of the subscriptions into one so it's easier to listen and unsubscribe to all of them.

It just feel like there's an operator for that.. maybe combineAll?

is toggle.change an observable?

Yes, it's an event that is fired when the selected state changes. We use merge for a couple of other similar cases as well (in the autocomplete and in the select once the multiple selection PR is merged).

EladBezalel · 2017-02-15T22:38:45Z

@crisbeto thanks!
@kara can you please review as well?

jelbourn · 2017-03-27T23:18:48Z

@crisbeto is this a breaking change for the toggle button API? If so, can you give a quick summary of the change?

crisbeto · 2017-03-28T07:10:15Z

It's not. It just gets rid of the need for having directives both for md-button-toggle-group and md-button-toggle-group[multiple].

kara · 2017-08-16T01:07:11Z

@crisbeto Is this still something we want to do? If so, can you rebase? Didn't catch this one because I wasn't assigned.

crisbeto · 2017-08-16T17:26:40Z

It does make sense, but looking at the amount of conflicts, it'll be easier to redo this from scratch.

…t in multiple mode * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as angular#2773, however angular#2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes angular#9058.

…t in multiple mode * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. * Switches all of the `async` button toggle tests to run in `fakeAsync`. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as angular#2773, however angular#2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes angular#9058.

crisbeto · 2018-01-02T13:45:50Z

Closing in favor of #9191.

…t in multiple mode * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. * Switches all of the `async` button toggle tests to run in `fakeAsync`. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as angular#2773, however angular#2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes angular#9058.

…t in multiple mode * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. * Switches all of the `async` button toggle tests to run in `fakeAsync`. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as #2773, however #2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes #9058.

…t in multiple mode * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. * Switches all of the `async` button toggle tests to run in `fakeAsync`. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as angular#2773, however angular#2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes angular#9058.

…t in multiple mode (#9191) * Reworks the `MatButtonToggleGroup` component to remove the need for the `MatButtonToggleGroupMultiple` component and to avoid having to implement features in two places. * Reworks the `MatButtonToggleGroup` to use the `SelectionModel` for managing its state. * Switches all of the `async` button toggle tests to run in `fakeAsync`. As a side-effect of the above-mentioned changes, the toggle group in multiple mode now supports the same inputs as the one in single-selection mode (`value` input/output, `name` input, `change` event etc.). Note: this is along the same lines as #2773, however #2773 got left behind for too long and it would need a lot more work to fit in our current setup. Fixes #9058.

angular-automatic-lock-bot · 2019-09-08T00:02:43Z

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.}

refactor(button-toggle): switch to SelectionModel

78639a0

* Switches the MdButtonToggleGroup to use the SelectionModel. * Avoids having to define two toggle group directives. * No longer fires change events from actions that weren't generated by the user.

crisbeto added the pr: needs review label Jan 23, 2017

crisbeto assigned jelbourn Jan 23, 2017

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 23, 2017

Fix AoT issues.

4cfa1fd

Merge branch 'master' into button-toggle-refactor

8e4e251

# Conflicts: # src/lib/button-toggle/button-toggle.html # src/lib/button-toggle/button-toggle.ts

EladBezalel reviewed Feb 15, 2017

View reviewed changes

EladBezalel requested a review from kara February 20, 2017 18:01

andrewseguin added pr: needs rebase and removed pr: needs review labels Mar 2, 2017

crisbeto mentioned this pull request Jun 14, 2017

refactor: replace disabled getters/setters with mixin #5137

Merged

kara removed their request for review August 16, 2017 01:07

kara assigned crisbeto Aug 16, 2017

jelbourn unassigned jelbourn and crisbeto Aug 23, 2017

crisbeto mentioned this pull request Jan 2, 2018

feat(button-toggle): general component cleanup and support value input in multiple mode #9191

Merged

crisbeto closed this Jan 2, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

refactor(button-toggle): switch to SelectionModel #2773

refactor(button-toggle): switch to SelectionModel #2773

Uh oh!

Conversation

crisbeto commented Jan 23, 2017

Uh oh!

EladBezalel commented Feb 15, 2017

Uh oh!

crisbeto commented Feb 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

crisbeto Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EladBezalel commented Feb 15, 2017

Uh oh!

jelbourn commented Mar 27, 2017

Uh oh!

crisbeto commented Mar 28, 2017

Uh oh!

kara commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto commented Aug 16, 2017

Uh oh!

crisbeto commented Jan 2, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

Uh oh!

crisbeto Feb 15, 2017 •

edited

Loading

kara commented Aug 16, 2017 •

edited

Loading