fix(selection-list): disabling list doesn't disable ripples of options #11955
Conversation
devversion
added
the
target: patch
googlebot
added
the
cla: yes
|
Why isn't this already captured by this getter? |
|
If the Apparently this does not mean that the child components itself will be updated because they also are set to So in that case we need to manually mark the options for check. e.g. through So in short: The getter will surely return the right value. But the value is not being used anywhere because the template of the |
src/lib/list/selection-list.ts
Outdated
| @@ -140,6 +142,9 @@ export class MatListOption extends _MatListOptionMixinBase | |||
| /** @docs-private */ | |||
| @Inject(forwardRef(() => MatSelectionList)) public selectionList: MatSelectionList) { | |||
| super(); | |||
|
|
|||
| this._parentStateSubscription = this.selectionList._stateChanges | |||
There was a problem hiding this comment.
Add a comment that explains what this is for?
Also, instead of keeping track of the subscription, we've been moving towards a pattern of having a _destroyed subject and using takeUntil
src/lib/list/selection-list.ts
Outdated
| @@ -335,6 +345,10 @@ export class MatSelectionList extends _MatSelectionListMixinBase implements Focu | |||
| }); | |||
| } | |||
|
|
|||
| ngOnChanges() { | |||
| this._stateChanges.next(); | |||
There was a problem hiding this comment.
Would it make sense to only do this for now in the disabled setter, or is there other state shared between the two components?
There was a problem hiding this comment.
Currently there is no other "state" that needs to be shared between child and parent.
Since we use the mixinDisabled, we can't customize the setter and notify the state change. As an alternative, we can just remove the mixin and implement the disabled property our self.
That way, we could even get rid of the stateChanges subject and just manually run something like options.forEach(option => option._markForCheck(). But maybe we will have things to share at some point. What do you prefer? (removing mixin seems to be the cleaner way for now)
There was a problem hiding this comment.
Yeah, I would go with removing the mixin for now.
There was a problem hiding this comment.
You can declare the changes: SimpleChanges parameter on the ngOnChanges method which will tell you whether disabled changed.
| it('should update state of options if list state has changed', () => { | ||
| // To verify that the template of the list options has been re-rendered after the disabled | ||
| // property of the selection list has been updated, the ripple directive can be used. | ||
| // Inspecting the host classes of the options doesn't work because those update as part |
There was a problem hiding this comment.
I'm not sure I get this part. Both the disabled class/aria-disabled and the ripple depend on the same property, which means that they should update at the same time, if they don't it could indicate that the test is catching another issue.
There was a problem hiding this comment.
The difference is: The host bindings of the <mat-list-option> seem to be updated as part of the parent template (<mat-selection-list>), while the actual template of the <mat-list-option> is only being updated if the change detection runs on MatListOption (which is obviously not the case)
Maybe I miss something crucial, but from my findings it looks like this is the case.
* Fixes that dynamically changing the `[disabled]` binding does not update the state of the list options (e.g. ripple disabled state; checkbox disabled styling) Fixes angular#9952
devversion
force-pushed
the
fix/list-state-change-options
branch
from
b3977d0 to
6fbd3e8
Compare
|
@jelbourn Done. Made the requested changes. |
jelbourn
added
pr: lgtm
action: merge
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
[disabled]binding does not update the state of the list options (e.g. ripple disabled state; checkbox disabled styling)Fixes #9952