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

Use template as el #2357

Closed
designermonkey opened this issue Feb 22, 2015 · 51 comments
Closed

Use template as el #2357

designermonkey opened this issue Feb 22, 2015 · 51 comments

Comments

@designermonkey
Copy link

One thing I find increasingly annoying about Backbone is it's penchant for wrapping everything in a div. While using LayoutManager for a short time, I liked how a developer could set the el of a View to false, so as to tell LayoutManager to use the provided template as the Views el.

IMO, this should be a part of Marionette also. There is already the option to pass false to template 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?

@samccone
Copy link
Member

xref
#2041
#815
#431
jashkenas/backbone#2329

I still stand by what I said in #2041

@designermonkey
Copy link
Author

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.

@samccone
Copy link
Member

Frameworks like this, need to be as agnostic about how a developer chooses to implement their HTML and CSS as possible

I agree 100%

I wrote up a blog post about why the wrapping div gives a bunch of power to event binding and such
http://blog.marionettejs.com/2015/02/12/understanding-the-event-hash/index.html

That said, you should take a stab at implementing a non wrapping version. I know some people have done this... But let me know

@designermonkey
Copy link
Author

Will do, I'm looking at it now.

@designermonkey
Copy link
Author

@samccone I just read your blog post. I thought that events were delegated to document until I read that.

I know I read a while ago that with jQuery, document was more performant at handling delegated events that delegating to loads of different nodes within the DOM. I also have read the opposite, so... Stumped.

If I were to use something like I am proposing, then I would more than likely have a CollectionView capture the events.

@samccone
Copy link
Member

I know I read a while ago that with jQuery, document was more performant at handling delegated events that delegating to loads of different nodes within the DOM. I also have read the opposite, so... Stumped.

Interesting! I did not know that, sound like you should perf it!

Looking forward to what you come up with :)

@jamesplease
Copy link
Member

@designermonkey, I think that you're exaggerating the problem here. Very rarely (never?) will the wrapping element create a situation where the wrapping div would mess up, say, an HTML parser. The list example you gave above (with the ul) would only ever happen if you are unaware of the tagName property of views.

As far as CSS goes, if a wrapping div messes up your CSS then you are potentially writing overly-specific CSS.

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.

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 divs in there!"

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.

If it can be proven to work in an extension of Backbone, then maybe it can be back-ported into Backbone.

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.

@designermonkey
Copy link
Author

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.

I would take the latter over the former any day of the week.

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.

@samccone
Copy link
Member

Like I said earlier, it's not Marionette's fault, but it can help fix it.

👍 👏

@ianmstew
Copy link
Member

@designermonkey Looks like we are finally solving this, just with a slightly different implementation, in 2.5. Instead of relying on the template to define the el, we are adding the option for a view's el to replace the region's el. That being the case I'm closing this, but please feel free to join the discussion over at #2625!

@jamesplease
Copy link
Member

I think this should stay open. It's a separate issue from regions

@ianmstew
Copy link
Member

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?

@ianmstew ianmstew reopened this Jun 29, 2015
@jamesplease
Copy link
Member

@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!

@ianmstew
Copy link
Member

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 tagName, className, id, and attributes in JS files.

@designermonkey
Copy link
Author

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.

@ianmstew
Copy link
Member

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?

@ahumphreys87
Copy link
Member

Im 👍 for this

On Mon, Jun 29, 2015 at 10:43 PM, Ian Stewart notifications@github.com
wrote:

Well, it sounds like LayoutManager probably has a great example for this.
@marionettejs/marionette-core
https://github.com/orgs/marionettejs/teams/marionette-core any concerns
before we start moving toward a PR here?


Reply to this email directly or view it on GitHub
#2357 (comment)
.

@paulfalgout
Copy link
Member

I think this will likely introduce some magic involving calling setElement after a render. It does seem doable, but we will want to be careful to not end up re-binding a bunch of things or not binding things that need to be bound.

@jridgewell
Copy link
Contributor

My only concern here is setElement is a very expensive method call, and getting even more expensive.

@samccone
Copy link
Member

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.

@megawac
Copy link
Member

megawac commented Jun 29, 2015

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

@paulfalgout
Copy link
Member

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 tagName and it's template like normal

@jamesplease
Copy link
Member

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 template: false.

@frankcortes
Copy link
Contributor

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.

@frankcortes
Copy link
Contributor

@jmeas, as a suggestion tagName: false doesn't mean anything for now.

@megawac
Copy link
Member

megawac commented Jun 29, 2015

Oh if its supplementary that would be awesome

@designermonkey
Copy link
Author

All you have to do is set your ItemView's tagName to li and remove the

  • s from your template. There's no problem here.

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

    @frankcortes
    Copy link
    Contributor

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

    @jridgewell
    Copy link
    Contributor

    I think there are two issues here:

    1. DOM attributes on the root node don't update after render
    2. The root node is defined in code instead of in a template

    Issue One

    The 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 li, but any later update's won't update the data-handle and id attributes. This can be easily fixed inside #render by updating id, class, and arbitrary attributes from #id, #className, and #attributes. Additionally we can support (though I wouldn't recommend) changing the root element's tag name.

    Issue Two

    The 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:

    1. UI elements are scoped to the root element
    2. DOM events attach to the root element, and that now happens on initialization for all views.

    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>

    @designermonkey
    Copy link
    Author

    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 ul>li, which does happen (and I don't want to hear answers like, stop them doing it. That's not very helpful).

    The <div/> other than being used for an events node is just cruft. I don't know how to solve it, and you may already be pointing in the right direction, but your answer so far @jridgewell so far seems like so much more work for me than just being allowed to declare that my el is my template (or at least a clone of it).

    (Thanks for responding with an idea though)

    @jamesplease
    Copy link
    Member

    @designermonkey, are you familiar with the tagName property of Views? Your root node can be an li.

    var View = Marionette.ItemView.extend({
      tagName: 'li'
    });

    @paulfalgout
    Copy link
    Member

    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 <li> from that template?

    @designermonkey
    Copy link
    Author

    @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 el when I should be able to declare it in my template?

    I should be able to say:

    Here View, here's a template node to use, just use it's root node for all your magical stuff and things.

    I should not have to say:

    Here View, here's a template to use, and also here's me having to write a load more JavaScript to recreate all that attribute stuff from the template's root node onto a node you just created for me, and then copy the contents of my template every time I call it, and dump it's perfectly defined root node, that you can already use, but prefer to wrap in another node you create every time I use you for some reason.

    Bonkers.

    @samccone
    Copy link
    Member

    We need to be clear here; this behavior comes directly from backbone and the ensureEl method.
    Changing this behavior would be a fundamental override, and a change to our current "hands off" approach with Backbone.View.

    If we think this is a good idea, we also have to realize that we will be changing the well rooted backbone view interface: id, attributes, className, tagName.

    What happens with these properties, do we just ignore them if someone does tagName: false?

    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.

    @jamiebuilds
    Copy link
    Member

    re: <dl><dt>...</dt><dd>...</dd></dl>

    Since this is the only example where view.el would be a problem I would much rather it suck for dealing with this edge case than change something so core to Backbone view. Just put the whole thing in your template and re-render the whole view, the performance implications are most likely tiny.

    @paulfalgout
    Copy link
    Member

    @designermonkey

    It has to be a single node, as you can't add multiple nodes into a template? It's in the documentation for Backbone.

    No I don't think this is true.. maybe you've read that about the <script> id being a single node, but for your original example this would work without issue:

    <script id="route-item" type="text/template">
      <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>
    </script>
    
    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.

    @jridgewell
    Copy link
    Contributor

    Related: Backbone rejected this feature a few years ago jashkenas/backbone#546. Of particular interest is Jeremy's response.

    @designermonkey
    Copy link
    Author

    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.

    @jamesplease
    Copy link
    Member

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

    @jamesplease jamesplease reopened this Jul 7, 2015
    @samccone
    Copy link
    Member

    samccone commented Jul 7, 2015

    @jmeas do you have a recommendation as to an approach here?

    @jridgewell
    Copy link
    Contributor

    Here's a minimal gist to accomplish template defined root elements.

    I'm going to open a PR against core Backbone for #_attributes to easily allow devs to update the root element's attributes after a render (my issue one above). Unfortunately, nothing else is appropriate for core, since rendering logic is a dev issue.

    @Tvrqvoise
    Copy link

    Wait a sec. Maybe I'm just not getting things here, but how would this be different from using the region as $el? If these two features accomplish the same end result, yet are incompatible and this one has the potential for serious performance impacts, I'm -1 for this feature.

    @designermonkey
    Copy link
    Author

    Because a region is a region. It is not a view. This behaviour is needed on the lowest level views.

    @ambischof
    Copy link

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

    @Tvrqvoise
    Copy link

    @ambishof, it looks like #2625 would provide that functionality.

    @hashchange
    Copy link
    Contributor

    hashchange commented Aug 16, 2016

    I have built a solution for Backbone which respects how the el is normally created – ie, early on, before initialize() is called. It plays nice with frameworks built on top of Backbone, like Marionette.

    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 setElement(). Have a look if you wish: Backbone.Inline.Template.

    The plugin solves two of the three issues discussed here:

    • Markup and Javascript are kept apart.
    • You can use the same templates, client-side and server-side, when your backend expects fully formed templates.

    The remaining problem which I didn't address:

    For that last one, there is no easy fix. I went with the "early el strategy". That early in the process, there is no generic, template-agnostic way to incorporate template variables for the el. (I can imagine extensions which would be able to handle that, but they would be specific to a project.)

    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.

    @paulfalgout
    Copy link
    Member

    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.

    @paulfalgout
    Copy link
    Member

    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.

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

    No branches or pull requests