Skip to content

Conversation

@Joelkang
Copy link
Contributor

@Joelkang Joelkang commented May 19, 2016

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:

  1. Is actionContext a legacy thing?
  2. Is there a better way of checking if something is a reference?
  3. This seems to be the only way to obtain the controller of the current outlet. Let me know if I'm way off base.
  4. For the privatization of the targetObject attr, I followed the aliasIdToElement() pattern, hopefully it's right

@Joelkang Joelkang force-pushed the target-actions branch 2 times, most recently from 62ed8b3 to dfeca6e Compare May 20, 2016 21:04
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this just bubble on it's own?

@homu
Copy link
Contributor

homu commented Jun 9, 2016

☔ The latest upstream changes (presumably #13621) made this pull request unmergeable. Please resolve the merge conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

This duplication seems odd. What is actually changed from the mixin in ember-views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@Joelkang Joelkang Jun 20, 2016

Choose a reason for hiding this comment

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

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

@homu
Copy link
Contributor

homu commented Jun 24, 2016

☔ The latest upstream changes (presumably #13745) made this pull request unmergeable. Please resolve the merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants