498 add event handlers #499
Conversation
| * @see https://github.com/cibernox/ember-basic-dropdown/issues/498 | ||
| * @memberof BasicDropdownContent | ||
| */ | ||
| @action |
There was a problem hiding this comment.
If you are not accessing this, there is no need of decorating the function with @action
# Conflicts: # addon/components/basic-dropdown-content.js
|
@cibernox I just rebased this PR on latest master. Since I originally created this PR, the repo has moved to TypeScript, which makes pinning this branch in our package.json more difficult as our project is not TS, so we don't have tooling setup to build a TS branch. With that in mind, I'm keen to pick up this PR and help to get it merged. So.... I have a question... What do you think about tests for this fix? It seems that proper integration tests would require mocking each of the interaction events to ensure that the callback is properly called. But since this is a stopgap solution, which will probably be un-documented and used by a small number of people and is pretty straight forward, I'm not sure it's worth the effort of adding tests. WDYT? |
|
@cibernox any thoughts on this, would be great to get it merged in so that our (non-TS) project can move to V3 & ember power select 4? Thanks! |
|
@cibernox I've added tests for the temporary event args and we're 🟢. Would really appreciate a review as this is blocking upgrading our project past Ember 3.16. Thanks! |
|
Merged, but this makes me a bit sad. Clearly the component helper should allow element modifiers to be passed in. |
|
Can you test this using master before I cut a new release? |
Me too, but I tried to make it easy to remove, so 🤞that something like emberjs/rfcs#497 ever lands.
We don't have TypeScript setup for our project, so it will take me some time to handle that. Is there another way to test, maybe cutting some temporary release? We've been using c839061 for at least 4 months in production now, and all that's changed since then is adding typings and tests... |
|
After discussing this on |
|
I've had a chance to test, it's working well. However, I noticed another related issue which I'll create a PR for on Monday |
|
@cibernox I've been using this branch for about a week now. We haven't done a release on master, but so far in my testing I haven't seen any issues. 👍 |
Closes #498.