fix(buttons): Fixes handling for disabled attribute and property #1095
Conversation
|
@pkozlowski-opensource Let me know what you think. It works, but I guess it could be done better. I had a look at Angular Material 2 for comparison and ideas. |
|
Added test and implemented the following:
I hope that this should fix all relevant functionality. |
| } | ||
|
|
||
| updateDisableState() { this._crf.detectChanges(); } |
There was a problem hiding this comment.
Wow, this sounds super-fishy! Why do we need to do this?
There was a problem hiding this comment.
I knew you'd mention that. I didn't like it either.
The question is what should happen when the group disabled state changes, say from true to false. The buttons should then check what state they are in. How should the buttons get notified when that happens?
I will have a look but let me know if you have any ideas.
| get disabled() { | ||
| let result = (this._group && this._group.disabled) || this._disabled; | ||
| if (this._label) { | ||
| this._label.disabled = result; |
There was a problem hiding this comment.
I'm not super-fan of having side-effects in a getter (in other words - we shouldn't be doing this). What is the motivation / problem that we are trying to solve here?
There was a problem hiding this comment.
I didn't like that either. It is a bit similar to the previous comment.
The question is how to keep the label in sync with the input. Again, I will have a look, but any suggestions appreciated.
|
@alex321 I had a look at the PR and while tests look good I've got some reservations regarding the existing implementation (left comments in the PR). Also I would prefer that we separate fixing a bug (handling of the Given the 2 above comments I'm not keen on merging this PR as-is. Do you think you can give it another shoot with just a bug fix and avoiding impl patterns I've pointed out? |
Scratch this, we do already have support for the disabled state on the group level. Sorry for the noise. |
Fixes #1088
Fixes #1089