-
Notifications
You must be signed in to change notification settings - Fork 319
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
Set the lit-html event context to this
and add a @eventOptions decorator
#244
Conversation
cc @katejeffreys and @arthurevans for docs. |
this
and add a @eventOptions decorator
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.
Code LGTM, but:
- Consider README updates above
- Fix tslint error, confirm CI
- Add to CHANGELOG
@@ -64,7 +64,7 @@ and renders declaratively using `lit-html`. | |||
* expression: ``` html`<div>${disabled ? 'Off' : 'On'}</div>` ``` | |||
* property: ``` html`<x-foo .bar="${bar}"></x-foo>` ``` | |||
* attribute: ``` html`<div class="${color} special"></div>` ``` | |||
* event handler: ``` html`<button @click="${(e) => this._clickHandler(e)}"></button>` ``` | |||
* event handler: ``` html`<button @click="${this._clickHandler}"></button>` ``` |
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.
Worth mentioning (here or elsewhere) that listener is bound to element (since that's not standard lit-html
)?
Also, worth showing a non-decorator pattern for options?
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.
Please show/add as many explanations and examples as possible on things like 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.
The README is about to be replaced with real docs.... It already didn't say how to set up event handlers correctly, so I think this at least isn't any worse.
Seconded (Edit: This was in reply to adams post in regard to examples/explanations) |
@kevinpschaaf PTAL. I didn't update the README yet because honestly I'm not sure where to put it, the README's far too long already, and it's about to be replaced. |
Oh, you already approved @kevinpschaaf :) |
Hah, oh well. I was going to suggest to modify the "declarative rendering" section along these lines:
|
Fixes #236 and #237
This lets users add methods as declarative event handlers without having to bind the method or use an arrow function. The decorator allows options to be set on the method for capture, passing and once.
This PR relies on lit/lit#531 and tests will fail till that is merged and released, though tests pass locally.