Updated paper-button #324
Updated paper-button #324
Conversation
| noSpan: Ember.computed('no-span', function() { | ||
| return this.get('no-span'); | ||
| }), | ||
| noSpan: computed.readOnly('no-span'), | ||
|
|
||
| // RippleMixin overrides |
There was a problem hiding this comment.
Remove this comment.
|
create the test:
|
| 'icon-button:md-icon-button', | ||
| 'focus:md-focused', | ||
| 'themed:md-default-theme', | ||
| 'themed:md-button' |
There was a problem hiding this comment.
We're adding md-default-theme by default in other components. e.g https://github.com/miguelcobain/ember-paper/blob/wip/v1.0.0/addon%2Fcomponents%2Fpaper-switch.js#L18
There was a problem hiding this comment.
Ok done. I also added paper-button class.
|
create the test:
|
| classNameBindings: [ | ||
| 'raised:md-raised', | ||
| 'icon-button:md-icon-button', | ||
| 'focus:md-focused', |
There was a problem hiding this comment.
this is pointless. BaseFocusable already has such a binding: https://github.com/miguelcobain/ember-paper/blob/wip/v1.0.0/addon%2Fcomponents%2Fbase-focusable.js#L14
Also, focus was renamed to focused. Anyway, this logic is no longer handled in paper-button.
|
Thanks for the PR! I've added some notes. Thanks. |
|
Thanks for the comments! Sorry I'm so slow, have had very busy days. I'll push the changes later today. |
|
Changes are up :) |
| 'raised:md-raised', | ||
| 'iconButton:md-icon-button', | ||
| 'themed:md-default-theme', | ||
| 'themed:md-button' |
There was a problem hiding this comment.
these classes should be hardcoded into classNames directly.
Remove themed property.
|
@jorgelainfiesta I've reviewed again. I think that is all. It will be an awesome button! 😄 |
|
@jorgelainfiesta ping |
|
Oh I forgot to push changes (facepalm) |
|
Oh apparently I only made the changes in my mind, sorry. Doing changes now, should take only a few mins. |
| this.sendAction('action', this.get('param'), event); | ||
| let onClick = this.get('onClick'); | ||
| if (isPresent(onClick)) { | ||
| this.get('onClick')(event); |
There was a problem hiding this comment.
Please use this.sendAction('onClick'):
- it is compatible with closure actions
- you don't need to check if an action exists that way
|
All changes are up :) |
| } | ||
|
|
||
| return this.get('bubbles'); | ||
| let onClick = this.get('onClick'); |
Hi,
Here's the PR for the update to
paper-button, jscs and tests are passing. I'll thank you all for the comments.Jorge L