Fix #75: md-button attaches class "md-undefined" to host element when on color provided #89
Conversation
googlebot
added
the
cla: yes
| HostBinding, | ||
| HostListener, | ||
| ChangeDetectionStrategy, | ||
| Renderer, |
There was a problem hiding this comment.
As mentioned in the style conventions, you should not indent with a tab, better indent with two spaces.
|
Two things; you need a proper constructor in MdAnchor to fix the Dartanalyzer errors. And you should squash your commits so this PR includes only one commit. I'm going to review the code later when I have some time. |
|
Thanks @hansl , still learning the first steps in contributing. Fixed the issues. |
| @@ -37,15 +55,15 @@ export class MdButton { | |||
| /** Whether a mousedown has occurred on this element in the last 100ms. */ | |||
| isMouseDown: boolean = false; | |||
|
|
|||
| setClassList() {return `md-${this.color}`;} | |||
There was a problem hiding this comment.
Why can this fix not be something like:
setClassList() {
return color != null ? `md-${this.color}` : '';
}?
There was a problem hiding this comment.
- This results in an empty class attribute which is not valid XHTML
- It overrides any other specified class in the component
<button md-button class="myClass" color="warn">Submit</button>
results in
<button md-button class="md-warn">Submit</button>
It should be
<button md-button class="myClass md-warn">Submit</button>
There was a problem hiding this comment.
I chatted with Misko and he agreed that this is an issue in Angular: angular/angular#7289
In the meantime I would be okay with the renderer approach.
|
This change needs to add unit tests that cover the bug. |
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
1 similar comment
|
We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm. |
googlebot
added
cla: no
|
CLAs look good, thanks! |
1 similar comment
|
CLAs look good, thanks! |
googlebot
added
cla: yes
… when on color provided fix(#75): fix md-button attaches class "md-undefined" to host element fix(#75): correct indent fix(#75): constructor in MdAnchor fix(#75): fix incorrectly extending base class fix(#75): fix Only static members can be accessed in initializers fix(#75): still learning :/ fix 'this' cannot be referenced in current location fix(#75): remove the previous color if one provided fix(#75): fixed messing up the inputs fix(#75): if value undefined, don't set element class fix(#75): removed unused testComponent fix(#75): replaced undefined with null because of dart
|
Two things:
|
|
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. |
No description provided.