Skip to content
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

3.4 Closure actions deprecation #973

Conversation

betocantu93
Copy link
Contributor

Deprecations Added in 3.4
Use closure actions instead of sendAction

https://emberjs.com/deprecations/v3.x#toc_ember-component-send-action

@avdept
Copy link

avdept commented Sep 4, 2018

Any progress on merging this?

@miguelcobain
Copy link
Collaborator

miguelcobain commented Sep 4, 2018

@betocantu93 Thank you very much for this.
Would it be too much to ask if we could use ember-invoke-action addon?

The addon has a functional utility, just like you made here, and also has a mixin. I'd prefer the functional utility because it is better aligned with ES6 classes which don't support mixins. I'm planning to migrate ember-paper soon.

Using this addon promotes using shared solutions and it is already tested against many ember versions.
Let me know if you're ok to do it.

@betocantu93
Copy link
Contributor Author

@miguelcobain Interesting, nice addon. Excellent news about ember-paper migration, let us know if you need help with that - also I was planning an attempt to get ember-paper run without any jquery - and sure, I can do the addon thing, I'll push to this branch I guess.

Copy link

@marcemira marcemira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link

@marcemira marcemira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a tiny change please 🙌 , nice work man!

@@ -170,7 +171,7 @@ export default Component.extend({

noUnselected(old, event) {
if (['Backspace', 'Delete', 'Del', 'ArrowLeft', 'Left', 'ArrowRight', 'Right'].includes(event.key)) {
this.sendAction('keyDown', event);
invokeAction(this, 'keyDown', item);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be event instead of item

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. This is the error on travis.
Linting tests are useful after all, huh.

@miguelcobain miguelcobain merged commit 8cf3ae9 into adopted-ember-addons:master Sep 5, 2018
@miguelcobain
Copy link
Collaborator

Thanks a lot for this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants