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

Allow Ember.View to set a default action for a target #1209

Closed
wants to merge 1 commit into from
Closed

Allow Ember.View to set a default action for a target #1209

wants to merge 1 commit into from

Conversation

leepfrog
Copy link

  • Modified ember-handlebars/lib/helpers/action (and tests) to include this behavior

Before Ember implemented the Router, the default target for an action was the View for a given template. This is no longer the case if a View has been created via the Router. Now, the Router is the default target.

What this means is that in order to target a View to handle an event, you need to specify {{action actionName target="concreteView"}}. This seems a bit odd (as the responsibility of the View class is to handle events).

This change will allow the View to become the default target if it specifies a method that matches the name of it's target. If the View does not specify a matching method name, it will perform the current behavior (and send it to the controller.target).

- Modified ember-handlebars/lib/helpers/action (and tests) to include this behavior
@wagenet
Copy link
Member

wagenet commented Jul 25, 2012

@wycats, @tomdale, I suspect you guys have opinions about this.

@visionscaper
Copy link

What are your plans related to this issue? It appears conceptually inconsistent to me as well : The action should be handled by the view in which it's defined, not by the router.

@lukemelia
Copy link
Member

Disagree. Events that will result in the state of the app changing (new route, model update, etc) should be sent to router for handling there. There are cases where it is more appropriate for a view to handle something, but in that case, you a) probably shouldn't be using an action helper, but rather a view that has a click handler, or b) you can set the target explicitly.

@visionscaper
Copy link

@lukemelia Oke, I guess it's a matter of taste, when handling the action, a view could explicitly ask the router to do it's thing when the state needs to change, instead of a build-in choice to give precedence to the router as handler.

In any case, could you explain how I can set the correct target? I tried target="concreteView" as @leepfrog suggested, "view" and the view name, but they all resulted in a handlebars error:

Uncaught Error: view doesn't match each handlebars-1.0.0.beta.6.js:553

I'm using Version: v1.0.pre-54-g3f7a118 of ember.js (currently the latest)

@mattkime
Copy link

give context="view" a try

@visionscaper
Copy link

@mattkime Thanks for the suggestion, but I get the same error. Could this be a handlebars issue?

@lukemelia
Copy link
Member

@visionscaper Here's a fiddle: http://jsfiddle.net/TgzwN/2/

@leepfrog
Copy link
Author

I agree with @lukemelia.

When I originally wrote this, I was thinking about the Ember.View as being a controller for all events. After a lot of back and forth (and some bashing into my head), I now understand that it's the Ember.ObjectController/Ember.ArrayController that should be responsible for handling view based events.

Only low-level events (such as click, etc..) should be implemented by the View. (Those are still delegated to the View for handling)

@mattkime
Copy link

thanks for the explanation, there is a fair amount of confusion here.

@lukemelia
Copy link
Member

I think there are two kinds of events.

  1. Events that affect local view state only.
  2. Events that affect non-local application state (models, controllers, route, etc).

IMO, events of type 1 should be handled in the view and events of type 2 should be handled by the router/stateManager.

@leepfrog
Copy link
Author

If it helps, here are some analogies to the Cocoa programming world:

Ember.View: UIView
Ember.ObjectController/Ember.ArrayController: UIViewController

@visionscaper
Copy link

@lukemelia Thanks for the Fiddle, that's the way I did it; it turned out to be another issue since I was doing two things at the same time ;) It works now. cc @mattkime

@visionscaper
Copy link

@lukemelia @leepfrog I agree with you both. However, I still think that the router should not be the default choice to handle the action:

  • (following @leepfrog 's line of thought) The controller should be the default handler, if something non-local needs to happen, the controller should approach the router for further handling;
  • As an alternative you should be able to set the target to the router if it concerns a state change, or
  • the view can be set as a target if it really only concern view dynamics (I consider this rare, the view should react to data it receives from the controller)

Cheers,
-- Freddy

@lukemelia
Copy link
Member

@visionscaper: @leepfrog and I discussed this with @wycats today. He described it in terms of layers of events. Views are responsible for turning DOM-oriented events into semantically meaningful application events that are sent to the router. One way of doing that is to use the Handlebars action helper, where you define the semantically meaningful action name and context, and it gets sent to the router. Otherwise, you handle events in the view and send along meaningful events to the router as appropriate.

You're free to have a different opinion, of course, and build your app best fits it, but the current behavior is intentional and thought through based on the above.

@visionscaper
Copy link

@lukemelia So, could I conclude that the router is a central hub, also responsible for forwarding application events to a controller? E.g. {{action getData}} => router.route::getData => controller::getTheDataAlready

@lukemelia
Copy link
Member

loading data typically happens on a transition and not an event, but as an example, I just added an action today like this:

feedUpdated: function(router, feed) {
  router.get('feedEntriesController').refreshFeedEntries();
} 

@visionscaper
Copy link

@lukemelia Yes, that's also how I've been doing it (assuming this method is defined in a route object). As I said, it wouldn't be my favoured approach but the most important thing for me is that the intended behaviour (and the thinking behind it) is now clear; I am capable of conforming to a standard ;)

Thanks for your time.

Cheers,
-- Freddy

@wagenet
Copy link
Member

wagenet commented Oct 8, 2012

@leepfrog It seems like this isn't in line with how we intend the router to be used. Feel free to argue more with @wycats if you want :)

@wagenet wagenet closed this Oct 8, 2012
rwjblue added a commit that referenced this pull request Nov 20, 2020
Includes the following bugfixes:

* [#1209](glimmerjs/glimmer-vm#1209) Ensure `<output form="some-value">` works properly
* [#1205](glimmerjs/glimmer-vm#1205) Ensure `@tracked` assertion can be made a deprecation
* [#1204](glimmerjs/glimmer-vm#1204) Ensure `loc` is populated by `build ers.element(...)`

Changelog here:

https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.66.1
rwjblue added a commit that referenced this pull request Nov 23, 2020
Includes the following bugfixes:

* [#1209](glimmerjs/glimmer-vm#1209) Ensure
  `<output form="some-value">` works properly
* [#1205](glimmerjs/glimmer-vm#1205) Ensure
  `@tracked` assertion can be made a deprecation

Changelog here:

https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.65.1
rwjblue added a commit that referenced this pull request Nov 23, 2020
Includes the following bugfixes:

* [#1209](glimmerjs/glimmer-vm#1209) Ensure `<output form="some-value">` works properly
* [#1205](glimmerjs/glimmer-vm#1205) Ensure `@tracked` assertion can be made a deprecation

Changelog here:

https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.62.5
sly7-7 pushed a commit to sly7-7/ember.js that referenced this pull request Mar 10, 2021
Includes the following bugfixes:

* [emberjs#1209](glimmerjs/glimmer-vm#1209) Ensure `<output form="some-value">` works properly
* [emberjs#1205](glimmerjs/glimmer-vm#1205) Ensure `@tracked` assertion can be made a deprecation
* [emberjs#1204](glimmerjs/glimmer-vm#1204) Ensure `loc` is populated by `build ers.element(...)`

Changelog here:

https://github.com/glimmerjs/glimmer-vm/releases/tag/v0.66.1
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.

5 participants