-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Use template as el #2357
Comments
xref I still stand by what I said in #2041 |
Thanks for replying so fast. After reading all those issues (thanks for the links, I did try looking), I can see how they are related. I am also astounded at some of the responses regarding why it should be accepted to allow arbitrarily wrapping anything added by JavaScript in a div, because, well, just because. I know this isn't a responsibility of Marionette, which goes a hell of a way to patch up the great simplicity of Backbone to be a useable framework for JavaScript, but I have to say, it is a problem that needs addressing. If it can be proven to work in an extension of Backbone, then maybe it can be back-ported into Backbone. Frameworks like this, need to be as agnostic about how a developer chooses to implement their HTML and CSS as possible. Personally, I don't tag every element with a classname because it looks horrible, and as stated in one response, it looks auto-generated, and doesn't adhere to DRY and KISS. Wrapping everything in divs forces a certain way of working, and when the JavaScript is the last piece of a very large project-puzzle to be done, having to go back through to re-write all of the CSS can ruin morale, budgets, deadlines, and client expectations, especially when the developers have written HTML and CSS in a very light manner to keep markup clean. Also, think about this markup... <ul>
<div>
<li></li>
</div>
<div>
<li></li>
</div>
...
</ul> You would never allow this to be output by a server side script, so it should never become acceptable for JavaScript. |
I agree 100% I wrote up a blog post about why the wrapping div gives a bunch of power to event binding and such That said, you should take a stab at implementing a non wrapping version. I know some people have done this... But let me know |
Will do, I'm looking at it now. |
@samccone I just read your blog post. I thought that events were delegated to I know I read a while ago that with jQuery, If I were to use something like I am proposing, then I would more than likely have a |
Interesting! I did not know that, sound like you should perf it! Looking forward to what you come up with :) |
@designermonkey, I think that you're exaggerating the problem here. Very rarely (never?) will the wrapping element create a situation where the wrapping As far as CSS goes, if a wrapping div messes up your CSS then you are potentially writing overly-specific CSS.
The reason that it's "allowable" is that many people think that exchanging a nice-to-have – HTML with no extraneous parts – for something that is actually useful – a Javascript library that does a lot of the heavy lifting for you, is worth it. I would take the latter over the former any day of the week. Nobody is looking at my websites saying, "You know, this is a really nice web application...but I wish there weren't so many unnecessary Fwiw, it's actually Regions that produce most of the unnecessary wrapping elements in my own apps, not views. I'm in favor of removing the wrapping elements – just because it's doable – but not because it solves the Javascript equivalent of world hunger.
I can say with a fair degree of confidence that it will never land in Backbone as long as the current team is in charge of the library. |
Not exaggerating at all, there's a real use case where it isn't helping at all. The template <script id="route-item" type="text/template">
<li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}">
<h2>{{ name }}</h2>
<dl class="stats">
<dt class="distance">Distance</dt>
<dd class="distance"><span class="value">{{ distance.value }}</span> <span class="unit">{{ distance.unit }}</span></dd>
<dt class="duration">Duration</dt>
<dd class="duration"><span class="value">
{{ duration.hours }}
{{ duration.minutes }}
</span> <span class="unit">{{ duration.unit }}</span></dd>
<dt class="attractions">Attractions</dt>
<dd class="attractions"><span class="value">{{ attractions.count }}</span> <span class="unit">{{ attractions.unit }}</span></dd>
</dl>
<nav>
<a href="/routes/{{ handle }}">
<i></i>
<span>View Route</span>
</a>
</nav>
</li>
</script> The View View = Backbone.Marionette.ItemView.extend({
tagName: 'li', // If I omit this, I get a wrapping div, or I get an extra wrapping li
template: '#route-item' // The template *must* be a single node, so content is already wrapped with an li
}); This just isn't right really. I've been doing this for nearly 15 years and have learnt one thing in my time: standards. HTML was standardised, CSS was standardised, JS is following suit. I just believe very strongly that all aspects of an application I and my team develop, should follow the same standards. But anyway, I'm not here to argue with you guys, and I never said anything about world hunger, so please don't push my concern to that proportion. I'm just trying to help contribute to an already great piece of software.
Last point, I totally agree with this, which is why I've been trying so many JS frameworks, and this is the only one I've found that follows good solid design principles. Like I said earlier, it's not Marionette's fault, but it can help fix it. |
👍 👏 |
@designermonkey Looks like we are finally solving this, just with a slightly different implementation, in |
I think this should stay open. It's a separate issue from regions |
Okay, I see your point. A better approach would be to ask the following: Given the ability to replace a region's element coming in #2625, where does this issue stand? |
@designermonkey makes a lot of good points, I think, and @tbranyen has successfully implemented this feature in LayoutManager. I think it'd be a huge win if we added this to Marionette! |
Hmm, it does solve a different problem and I think you're right... it would be an instant fan-favorite. It would allow designers to write wrapper-less HTML without need editing |
One of the other added benefits would be server side markup matching templates too. I wish I had the time to contribute to open source more to help achieve this. |
Well, it sounds like LayoutManager probably has a great example for this. @marionettejs/marionette-core any concerns before we start moving toward a PR here? |
Im 👍 for this On Mon, Jun 29, 2015 at 10:43 PM, Ian Stewart [email protected]
|
I think this will likely introduce some magic involving calling |
My only concern here is |
We should keep in mind marionette is an extension lib not a fit all purpose framework. Whatever decision we end up with it should follow this thinking. |
Another thing to consider is this will likely break many (almost every?, all the major ones I've ever worked on) applications currently in Marionette and be a pain to upgrade |
Hmm is this a change or an addition? As I understood it, it would be some new view configuration, but any current view would go one using |
This wouldn't change the default behavior, so it wouldn't break anyone's apps: it would be an option that you must configure, sorta like |
I'm only talking about my own experience. I have implemented/watched Marionette in very different projects, and one of the first things that the team notices about the implementation is always the same. Although Marionette is Backbone-based - so it should extend and not replace the basic functionality - , I agree with @designermonkey's motivation. In my view, Marionette should fill what Backbone lacks. |
@jmeas, as a suggestion |
Oh if its supplementary that would be awesome |
@thejameskyle but yes there is, as I then can't use a template, as a template requires a single node. I present the example above again. <script id="route-item" type="text/template">
<li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}">
<h2>{{ name }}</h2>
<dl class="stats">
<dt class="distance">Distance</dt>
<dd class="distance"><span class="value">{{ distance.value }}</span> <span class="unit">{{ distance.unit }}</span></dd>
<dt class="duration">Duration</dt>
<dd class="duration"><span class="value">
{{ duration.hours }}
{{ duration.minutes }}
</span> <span class="unit">{{ duration.unit }}</span></dd>
<dt class="attractions">Attractions</dt>
<dd class="attractions"><span class="value">{{ attractions.count }}</span> <span class="unit">{{ attractions.unit }}</span></dd>
</dl>
<nav>
<a href="/routes/{{ handle }}">
<i></i>
<span>View Route</span>
</a>
</nav>
</li>
</script> I've read soooo many SO posts, blog posts, twitter posts all complaining about how JS frameworks wrap in divs, so there is obviously a perceived problem here. |
@thejameskyle Since you must have part of your template (the container element) inside of your code, it could be considered as an issue. I understand the reason behind doing this but it should have an alternative. |
I think there are two issues here:
Issue OneThe first one is easily solvable while still following core Backbone, and I think might actually solve @designermonkey's problem (correct me if I'm wrong). <li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}"> If that's the root node we're trying to create using core Backbone, I'd write it like so: class View extends Backbone.View {
attributes() {
return {
data-handle: this.model.get('handle')
}
}
id() {
return `entry-${this.model.get('id')}`;
}
className() {
return 'route item';
}
tagName() {
return 'li';
}
} Initialization will perfectly create my root Issue TwoThe second issue is what I see being talked about in this thread. That is, defining both content and the root element inside the template. Like I've already pointed out, this is going to help people write some very slow renders. There are two performance issues with this approach:
Perf Issue one can be completely mitigated if we assume our users aren't idiots (they aren't defining multiple "root" elements in the template). {{! Good }}
<li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}"></li> {{! Bad }}
<li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}"></li>
<span>Multiple root elements!</span> Perf Issue two can be mitigated only after the initial render by solving Issue One (not perf issue one). If we just update the view's content with everything inside the template defined "root" element and throw away the template root. But there's no helping the initial render, it's just gonna be damn slow. {{! Gonna throw the root away on additional renders }}
<li class="route item" id="entry-{{ id }}" data-handle="{{ handle }}">
{{! And just grab all it's content }}
<span>Content</span>
</li> |
The issue I have is simply the wrapping. I generate initial content from the server as it's sooo much quicker, then get pissed off that all new content from models and views is wrapped by divs. <ul>
<li>Already</li>
<li>From</li>
<li>Server</li>
<div>
<li>Generated</li>
</div>
<div>
<li>From</li>
</div>
<div>
<li>Javascript</li>
</div>
</ul> Now, this can cause issues when a developer has followed web standards expectations of semantic markup and targeted with css like The (Thanks for responding with an idea though) |
@designermonkey, are you familiar with the var View = Marionette.ItemView.extend({
tagName: 'li'
}); |
We're going in circles a bit here #2357 (comment) However in that example I'm not sure why "The template must be a single node" @designermonkey why can't you remove the |
@jmeas, as previously posted View = Backbone.Marionette.ItemView.extend({
tagName: 'li', // If I omit this, I get a wrapping div, or I get an extra wrapping li
template: '#route-item' // The template *must* be a single node, so content is already wrapped with an li
}); @paulfalgout it has to be a single node, as you can't add multiple nodes into a template? It's in the documentation for Backbone. Also, why, when I have a template language at my disposal to fill in properties, must I write a load more code per View to assign attributes to a View's I should be able to say:
I should not have to say:
Bonkers. |
We need to be clear here; this behavior comes directly from backbone and the If we think this is a good idea, we also have to realize that we will be changing the well rooted backbone view interface: What happens with these properties, do we just ignore them if someone does I would reiterate what I have said before, Marionette is an extension library. I can see someone making a standalone "extended" view layer that has this behavior in an isolated repo, when they feel like they have something working in a sane way that also passes Marionettes tests we can evaluate it (if we think it is a general usecase). In the end however this is a backbone thing, I would rather see this land in Backbone than in Marionette. |
re: Since this is the only example where |
No I don't think this is true.. maybe you've read that about the
View = Backbone.Marionette.ItemView.extend({
className: 'route item',
id: function() {
return this.model.id;
},
attributes: function() {
return {
'data-handler': this.model.get('handler')
};
},
tagName: 'li',
template: '#route-item'
}); Is there issue with this solution? I really think there are very few HTML issues that can't be resolved with Bb + Mn. But from a complexity and having DOM stuff on the view, it would be nice, however likely not practical for the root to be derived from the template.. However I don't think it is necessary. |
Related: Backbone rejected this feature a few years ago jashkenas/backbone#546. Of particular interest is Jeremy's response. |
I'm very close to giving up on this whole thing as it seems I can't seem to make people understand what is so fundamentally wrong with div wrapping. Also, the solutions proposed do indeed work, but they have to be considered a hack. Why? Because details I can and should describe in my template are having to be pulled across into JavaScript, therefore separating out my single template into two places. Just because the current idea is to use it as an event target for binding, and just because a template can indeed have more than one sub-node, and just because you can re-create my example in JavaScript and a template working together doesn't mean it is right. I've not been asking for us to make any fundamental changes to the way Backbone is expected to function, or monkey-patch it permanently from Marionette's perspective of use; I just proposed that Marionette become more agnostic and give developers an option to use the template node. Anyway. I give up. |
@designermonkey, I'm sorry that you're frustrated! I understand that it can be annoying to try to prove your point. But I'm on your side here, so I'm gonna reopen this - it's worth thinking more about. Do note that if you don't want to follow along you can unsubscribe to the right. |
@jmeas do you have a recommendation as to an approach here? |
Here's a minimal gist to accomplish template defined root elements. I'm going to open a PR against core Backbone for |
Wait a sec. Maybe I'm just not getting things here, but how would this be different from using the region as |
Because a region is a region. It is not a view. This behaviour is needed on the lowest level views. |
@Tvrqvoise I myself have come across this solution, however inelegant. It would be really nice if region el s could be leveraged similarly to a CompositeView's childViewContainer, however then you either have to rewrite the region's show/attachHtml code or manually bind the currentView to the view and then all your event bindings have to be manually .. and all the native region events don't... Excuse me while I have a flashback... I guess my point is, this is totally a doable option but right now too much of the region's core functionality is too tightly embedded in the show function for this to be a no-brainer solution. If you've come up with a SIMPLE way to use a region's el as the view el that doesn't conflict native features, I'm all ears. |
@ambishof, it looks like #2625 would provide that functionality. |
I have built a solution for Backbone which respects how the You can drop it into an existing project without changing a thing, and then turn it on for individual templates on a case-by-case basis. There isn't any negative impact on performance, unlike an approach based on The plugin solves two of the three issues discussed here:
The remaining problem which I didn't address:
For that last one, there is no easy fix. I went with the "early So the plugin doesn't solve the problem for everyone, but I suppose it might be helpful for at least some of the people in this thread. |
If anyone wants to weigh in on this issue #3259 is a possible solution. Beyond that, I don't know that there's a good way to accomplish this outside of removing Backbone. |
I'm pretty confident this will be settled using custom renderers in the future and should not be an option on the view. The user will have to be aware per render on how it will interact with default backbone.view behavior, but it won't be Marionette itself modifying this. We can re-open if someone disagrees. |
One thing I find increasingly annoying about Backbone is it's penchant for wrapping everything in a
div
. While usingLayoutManager
for a short time, I liked how a developer could set theel
of aView
tofalse
, so as to tellLayoutManager
to use the provided template as theView
sel
.IMO, this should be a part of
Marionette
also. There is already the option to pass false totemplate
to disable it, so would follow suit in that regard.It would be optional and defaulted to the current behaviour to not break any backwards compatibility.
From what I can see of the source code, it's not easily something that can be monkey patched in an extension of
View
either, although I am really going to try.Any thoughts?
The text was updated successfully, but these errors were encountered: