[Glimmer2] Add support for target actions and move related ember-view tests to new harness #13532
Conversation
Joelkang
force-pushed
the
target-actions
branch
2 times, most recently
from
62ed8b3
to
dfeca6e
Compare
| let action = this.actions && this.actions[actionName]; | ||
|
|
||
| if (action) { | ||
| var shouldBubble = action.apply(this, args) === true; |
There was a problem hiding this comment.
Does this just bubble on it's own?
|
☔ The latest upstream changes (presumably #13621) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -0,0 +1,217 @@ | |||
| import { Mixin } from 'ember-metal/mixin'; | |||
There was a problem hiding this comment.
This duplication seems odd. What is actually changed from the mixin in ember-views?
There was a problem hiding this comment.
It is indeed, the mixin in ember views actually only provided the TargetActionSupport mixin and a couple of CPs. Most of the actual methods for targetable actions in views was implemented in ember-htmlbars/lib/component.js. It made sense to me to actually unify these implementations into its own mixin and not pollute the component.js file for glimmer.
There was a problem hiding this comment.
I agree with the intent, but I would rather see the implementation moved into the "real" target-action-support.js. Every file we add to packages/ember-glimmer/lib/<some other package name>/lib/ must be moved before we ship, and assuming the implementation here is the same as ember-htmlbars/component there is no reason to make our lives harder...
There was a problem hiding this comment.
the only key difference is in the way the action is resolved either from attrs. or the ARGS symbol on the component.
That said, I get what you're saying, but I'm not sure how do make the change without modifying the API of the old view_target_action_support.js mixin. If I move the send() and sendAction() into target-action-support.js then view_target_action_support will also have those methods, even though in glimmer1 they're only added at the component.js level, where view_target_action_support is mixed-in.
This means that anyone manually mixing-in view_target_action_support.js will get more than they bargained for. If this is fine though, I'll go ahead and do that
…test since it is private
… tests to properly expect it
|
☔ The latest upstream changes (presumably #13745) made this pull request unmergeable. Please resolve the merge conflicts. |
I didn't delete the tests in the ember-view package since it's not clear to me that we can symlink them the way we did for the ember-htmlbars tests.
Some open questions:
targetObjectattr, I followed thealiasIdToElement()pattern, hopefully it's right