-
-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ripples and icons #275
ripples and icons #275
Conversation
…er type of mixin (like Angular material does it). Fixes ripple on checkbox and button
@peec I appreciate the effort. Isn't a ripple service an overkill? A service should be used whenever there is a shared state. What shared state is there to hold in ripple effects? |
@miguelcobain True, there is no shared state. I figured that as a service the ripple effect can be used on DOM elements separately without any need of a component and multiple times. E.g: didInsertElement() {
this.get('ripple').attach(this.$('.some-item-one'));
this.get('ripple').attach(this.$('.some-item-two'));
} So basically it's completely separated from the component, rather bound to a DOM node. But then again, I understand that it might be overkill. |
I don't see why that wouldn't be possible without a service. didInsertElement() {
this.attachRipple(this.$('.some-item-one'));
this.attachRipple(this.$('.some-item-two'));
}
|
I still like the 1 - This would attach ripple to export default Ember.Component.extend(RippleMixin, {
}); 2 - This would attach ripple to export default Ember.Component.extend(RippleMixin, {
rippleContainerSelector: '.inner-selector'
}); 3 - This would attach ripple to export default Ember.Component.extend(RippleMixin, {
rippleContainerSelector: ['.inner-selector', '.another-selector']
}); 4 - This would not attach any ripple, basically turning it into the "manual" mode. This means the component including the mixin will have to manually call export default Ember.Component.extend(RippleMixin, {
rippleContainerSelector: null,
didInsertElement() {
//this is a silly example, because it would be better to use case 3.
//imagine that some-item-one and some-item-two are dynamically generated or something
this.attachRipple(this.$('.some-item-one'));
this.attachRipple(this.$('.some-item-two'));
}
}); I think these guidelines would make it a really flexible mixin! |
@miguelcobain Good points. Moved from service to ripple-mixin. |
height: size + 'px' | ||
}; | ||
createContainer() { | ||
var container = Ember.$('<div class="md-ripple-container"></div>'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're now using let
for variables and const
for constants.
@peec I'd prefer to eliminate the intermediate RippleMixins you created and have the properties on the component itself. Otherwise we have things like importing You have any objections? |
@miguelcobain Agreed. I updated and now it's just ripple-mixin along with optimizations for code structure. |
|
||
return true; | ||
}, | ||
inkRipple() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since inkRipple()
is basically a wrapper around rippleInk
computed, we can replace all inkRipple()
entries with this.get('rippleInk')
.
Why did we remove those tests in |
@@ -28,7 +28,7 @@ | |||
{{#paper-subheader class="md-no-sticky"}}Clickable Items with Secondary Controls{{/paper-subheader}} | |||
|
|||
{{#paper-item action="transitionToWifiMenu"}} | |||
{{paper-icon "network-wifi"}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did these happen?
Did the stylesheets change icon names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://github.com/google/material-design-icons/blob/master/iconfont/codepoints , this is valid names. I could not get it working with dashes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. The icon names have underscores.
I wonder why we had dashes before. Did material icons change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think something changed but not sure where, you can also try here:
- http://jsbin.com/rifotulaja/edit?html,css,output
Change the underscores to dashes and it will break the icon,
ref. Multi-word icon names not well documented google/material-design-icons#177
@miguelcobain tests now passes, i think its ready. |
@peec only thing missing is to update the icon name list in the docs at Alternatively we could just point users to the material icons page for that list. Thoughts? |
@miguelcobain I actually updated that list, but its too large for github to see here. https://github.com/peec/ember-paper/blob/wip/v1.0.0/tests/dummy/app/controllers/icons.js List is directly from https://github.com/google/material-design-icons/blob/master/iconfont/codepoints Ill merge this then |
Ah, sorry. I didn't see that. |
Fixes (1.0 wip):