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

Implemented Spine-like elements configuration #569

Closed
wants to merge 1 commit into from
Closed

Implemented Spine-like elements configuration #569

wants to merge 1 commit into from

Conversation

bruth
Copy link

@bruth bruth commented Aug 18, 2011

Implementation of this feature from Spine: http://maccman.github.com/spine/#s-controllers-elements

Reopened from #511 that has been rebased with master.

@bruth
Copy link
Author

bruth commented Aug 18, 2011

This is still up for discussion since #511 (comment) pointed out the potential issue of not having this.el be available (which would result in the attributes not referencing anything), but I think it depends on the intended usage of the feature. For example, the below selector references an element in the DOM that may have child elements, thus the elements property would make sense.

...
el: '#foo'
elements: {
    'h1': 'title',
    'h4': 'subtitle'
}
...

If dependent on the View to create the element for you using, id, tagName, etc. then this would clearly not make sense. For people who use templates, typically that logic is put in the render method:

...
template: _.template('<hgroup><h1><%= this.title %></h1><h4><%= this.subtitle %></h4></hgroup>'),
render: function() {
    this.el = this.template(this.model.toJSON());
    this.setLocalElements();
}
...

@keithamus
Copy link
Contributor

A huge +1 for this, in our Backbone app we are using class level properties suffixed with tagName to generate/delegate to elements other than this.el. This would be a fantastic improvement to our setup.

@javisantana
Copy link

+1

@timoxley
Copy link

+2

@geddski
Copy link

geddski commented Aug 23, 2011

+1

@bruth
Copy link
Author

bruth commented Sep 3, 2011

bump for merging. there seems to be a decent following.

@fellix
Copy link

fellix commented Sep 14, 2011

+1

@keithamus
Copy link
Contributor

Worth pointing out that this issue marries up nicely to issue #374's goal of implementing the more useful features from Spine.js

bruth added a commit to chop-dbhi/cilantro that referenced this pull request Oct 13, 2011
Includes:
- jashkenas/backbone#569 ability to define local elements
- jashkenas/backbone#446 integrate Collection.update
- jashkenas/backbone#570 ability to pass a single attr/value pair in `set' and `save'
- jashkenas/backbone#567 this.el being the jQuery/Zepto object
@jgoldberg
Copy link

+1

@jashkenas
Copy link
Owner

There are a couple of potential problems here ...

this.el will often not be available (or correct) until after initialize runs.

Even if you wait that long, the elements probably won't be available until after render runs. Even if you wait that long, not every element will be available in every render. (Or you may not need all of them).

Really, this is a premature optimization. You should be using the very tightly scoped this.$(selector) to find elements within your view ... and if you're doing it repeatedly, cache the element in a local or instance variable at the appropriate time. Even if we got the above concerns to be bulletproof, I still think this would lead to heavy over-traversal of the DOM at view creation/render time -- when things are already tight enough -- instead of on-demand, later on during an event.

@jashkenas jashkenas closed this Jan 17, 2012
@marcalj
Copy link

marcalj commented Jan 17, 2012

+1

@bruth
Copy link
Author

bruth commented Jan 17, 2012

@jashkenas Your concern regarding when this.el will be available is assuming the default behavior of Backbone.View is not used. I realize it does not account for the universe of scenarios, but the way this.el is expected to be created using _ensureElement gives this implementation utility.

All your concerns regarding overriding the render() method and altering (or overriding) this.el is on point with potential issues with delegateEvents() as well. Only the developer knows when this.el will be ready, so they should be expected to call any post-setup methods at the appropriate time.

I don't understand why you mention that I should be using this.$(selector) and caching the elements? I do, and this is mostly the point of the patch. Line 997 uses this.$(selector) and sets the elements locally on the view instance.

@jashkenas
Copy link
Owner

The point of having a root "el" and using event delegation is that events can be delegated before the UI is rendered and their elements actually exist. The same is not true for jQuery selectors. You'll end up with a bunch of empty jQuery objects if setLocalElements runs before render does: the common case.

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

Successfully merging this pull request may close these issues.

None yet

9 participants