-
Notifications
You must be signed in to change notification settings - Fork 2.4k
How to declare simple observer with function #2436
Conversation
@arthurevans and/or @TimvdLippe can has sanity-check? |
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.
Overall LGTM. Just a question on a potentially nicer notation of the function declaration, but it is mostly a nit 😛
app/2.0/docs/devguide/observers.md
Outdated
observer: | ||
function(newValue, oldValue){ | ||
this.toggleClass('highlight', newValue); | ||
}.bind(this) |
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 a lambda reads clearer. Does the following work?
observer(newValue, oldValue) {
this.toggleClass('highlight', newValue);
}
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.
So this?
observer:
function observer(newValue, oldValue) {
this.toggleClass('highlight', newValue);
}
Or ...?
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.
Like this:
static get properties() {
return {
active: {
type: Boolean,
// Observer method
observer(newValue, oldValue) {
this.toggleClass('highlight', newValue);
}
}
}
A running example: http://jsbin.com/xisifaboso/edit?html,console,output This is using the "object method shorthand definition" in ES2015: https://developer.mozilla.org/nl/docs/Web/JavaScript/Reference/Functions/Method_definitions
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.
Cool, didn't know that was a thing XD
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.
Yeah it is fairly new and neatly concise 😄 This PR now LGTM fully!
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.
Great, thanks!
Fixes #2404