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

Store data-attributes with template. #318

Closed
wants to merge 2 commits into from
Closed

Store data-attributes with template. #318

wants to merge 2 commits into from

Conversation

busyloop
Copy link

For named templates this stores the data-attributes of the
<script>-tag in the 'data_attr'-property of the generated template.

Previously any custom attributes set by the user would be lost as Handlebars.bootstrap remove()s the template-elements during initialization. There are however scenarios where annotating the templates with custom attributes is very useful (I'm using them to add routing hints), thus this patch to retain them.

The test-suite passes. I didn't write a specific test yet because of #317.

For named templates this stores the data-attributes of the
<script>-tag in the 'data_attr'-property of the generated template.
@wagenet
Copy link
Member

wagenet commented Jan 2, 2012

One small comment, shouldn't it be dataAttr not data_attr to keep with standard JS naming conventions?

@busyloop
Copy link
Author

busyloop commented Jan 2, 2012

@wagenet You are right, fixed.

@pangratz
Copy link
Member

pangratz commented Jan 2, 2012

As stated by @busyloop in #317 (comment) this issue is related to #299. So should I merge in those changes and generalize the pull request #299 by adding support for this one ... ?

@wagenet
Copy link
Member

wagenet commented Jan 5, 2012

Looks like #299 got merged. However, if this is a good idea, then we should probably refactor some of the code from #299.

@pangratz
Copy link
Member

pangratz commented Jan 5, 2012

In what way? Adding the data attributes as property to the template and set all view related properties (like tagName, ...) in the data attributes on the view?

@busyloop
Copy link
Author

busyloop commented Jan 5, 2012

@pangratz That would be my thinking, yes. My patch doesn't go that far (it only attaches them to the Template for a start) but in principle they should imho either override or serve as default for the View-attributes.

@pangratz
Copy link
Member

pangratz commented Jan 5, 2012

Ok, sounds good. @wagenet do you agree?

I wanted to extend the test cases so all different script types are tested anyway. I would merge your patch and write tests too. Are you ok with this, @busyloop ?

@busyloop
Copy link
Author

busyloop commented Jan 5, 2012

Yup, sounds great!

@ghost ghost assigned wycats Jan 17, 2012
@wagenet
Copy link
Member

wagenet commented Jan 25, 2012

@pangratz, @busyloop: what's the status of this?

@busyloop
Copy link
Author

I've not been working on this since pangratz said he plans to merge it.

@pangratz: Any news?

@tomdale
Copy link
Member

tomdale commented Jan 25, 2012

Can someone explain the use case of this for me?

@busyloop
Copy link
Author

@tomdale

My use-case is to annotate the view-templates with their route, such as:

%script(type='text/x-handlebars' data-template-name='modules'
data-route='/app/modules' data-label='Modules Overview')

This is obviously app-specific; my app implements routing by setting all views that have a matching 'data-route' to visible on url-change, and all remaining views to invisible. This turns out to be an awfully convenient way to do the routing, and having the route directly on the template is the natural way to go in this approach (so stuff can be shifted around without having to bother with any external route -> view mappings).

My belly-feeling is there are more cases where having these attributes stick around can be useful (e.g. to generalize #299), but that's the one that I really care about right now.

@tomdale
Copy link
Member

tomdale commented Jan 26, 2012

I don't think we can accept this pull request. I want to avoid the inline script shorthand becoming an entire second system for defining views. In general, our philosophy is to keep as much truth out of the DOM as possible, and move it to JavaScript.

Especially in this implementation, copying the values on to the template itself seems wrong. I'm curious to know how your routing system is even finding this data (iterating over all templates? views?) but there are probably much more elegant ways to handle this on the JavaScript side. My approach would be to name the templates using the data-template-name attribute, then show and hide views with that templateName when routes were entered. In my app, I'm using ViewState to do that.

@tomdale tomdale closed this Jan 26, 2012
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 8, 2018
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 8, 2018
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 9, 2018
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 10, 2018
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 10, 2018
josemarluedke added a commit to josemarluedke/ember.js that referenced this pull request Oct 12, 2018
rwjblue added a commit that referenced this pull request Oct 13, 2018
[FEATURE array-helper] Implement array helper RFC #318
sandstrom pushed a commit to sandstrom/ember.js that referenced this pull request Jun 17, 2021
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.

6 participants