Subview replaces container rather than become child

[mikehedman]

The docs state:

container {String} Selector that describes the element within the view that should hold the subview
To me, that indicates that the container will remain, but have a new child once the subview is rendered. But this is not happening. Instead, the action() function in _parseSubview() calls render() which calls renderWithTemplate(), which then replaces the container with the subview:

var parent = this.el && this.el.parentNode;
if (parent) parent.replaceChild(newDom, this.el);

Here's a sample view that illustrates the issue:

var View = require('ampersand-view');
module.exports = View.extend({
    pageTitle: 'tester',
    template:[
        '<div class="root">',
            '<div class="one">',
                '<div class="two"></div>',
            '</div>',
        '</div>'
    ].join(''),
    subviews: {
        whoWeAre: {
            container: '.two',
            constructor: View.extend({
                template: '<p>Mike</p>'
            })
        },
        whatWeDo: {
            container: '.two',
            constructor: View.extend({
                template: '<p>Write code</p>'
            })
        }
    }
});

The intent is to build a page with a series of subviews that all get put into the same container. But the resulting html is not what one would expect:

<div class="root active">
  <div class="one">
    <p>Mike</p>
  </div>
</div>

Rather than my container div still being there with two children, instead only the first child exists, and the second child does not get rendered because it can't find its parent.

I think the renderSubview() function has the right idea - it uses appendChild rather than replaceChild. But re-routing that action() function to use renderSubview didn't work for me because it eventually calls renderWithTemplate - and since it can find a parent it calls replaceChild.

[simme]

I think that if the subview hash contains a container property the subview should be automatically rendered and appended to the given container (if it exists).

[HenrikJoreteg]

first off, sorry this has been sitting for so long. I agree that it's confusing the way it's written and that it's a problem.

The original thought was that the parent view could basically insert some DOM elements as placeholders. Since when you renderWithTemplate in a view it replaces its own root, the container was simply handed to the view element as its .el and the subview would then be able to take over, and be able to know where to render itself within the parent without having to leave a bunch of otherwise worthless wrapper elements around. I've always been a fan of not excessively wrapping things in elements and keeping the DOM minimal.

In that I never considered the use case of wanting to push to subviews into the same container without them being part of a collection.

In this particular case, since you have a known number of subviews, couldn't you simply create placeholder elements for each subview, with different ID or data-hook.

I guess what I'm asking is, if you think we should change the behavior or the docs and the name container. It sounds like you're saying change the behavior.

[simme]

I hear you and I do largely agree.

Creating placeholders for each subview adds an additional and unnecessary maintenance burden (Want to add another subview in a container? Go add another placeholder too!).

In my opinion container suggests that the view will be put inside of the specified container. So I'm proposing a (breaking I guess) change that makes selector and hook replace the given DOM node and container puts the view inside it. Maybe add another property wrapper if you want to keep breaking changes to a minimum, add a note in the docs about container changing behavior in the future and switch it in the next major release. Although, it's a fairly simple thing to change in ones code ;)

Now, it might be confusing to have two behaviors for inserting subviews. But I think it makes sense and the documentation could be fairly straightforward. It seems to me though that subviews overall is kind of a confusing topic at the moment! So straightening things out a bit is probably a good idea :)

[mikehedman]

@HenrikJoreteg, to answer your question, I would suggest that the behavior should be changed to be consistent with how the term "container" is used throughout the framework. For me, I like to take the long term view - that is, the framework is still young, so it's best to tighten things up now so that people aren't stumbling on confusing bits 5 years later.
That being said, I personally have no problem with putting placeholder divs as placeholders for where subviews should be placed - I just don't think those placeholders should be called "container" if they will be replaced by the subview.

[bryanspears]

My confusion stemmed from the fact that renderSubview() functions differently than the subviews hash. The renderSubview() method appends to container/this.el where subview replaces.

I'm not sure I mind which way the decision turns, but please have it be consistent across all subview handling code.

[cdaringe]

i've had to design around this as well. it's definitely do-able. in my case, i've wanted to bind listeners to an element that 💨 (whoosh) disappears 💨 once I render a view on it. as a consequence, in a my parent's view definition, I then need to know about my child view's hooks to bind the parent view's desired handlers. i like the container suggestion.

my workaround (ref'ing child hooks) could admittedly be an anti-pattern. such listeners perhaps could be bound to my child view's el within the child view definition, vs. the parent definition. those listeners would have to trigger an event for the parent to listen to such that i could achieve the same behavior. thinking out loud, this is now a weak point in that there's still an extra implicit view-relationship to manage between parent-child.

container sounds more & more appealing as it keeps my binding, my template, and my behavior in the same place. this is how we did it in forms for specifying a pre-existing FieldView container, @HenrikJoreteg, at your own suggestion. Albeit we still used el as a target container.

[bryanspears]

Could this please get some more attention? It's odd to have the subviews hash and renderSubview() work differently. Muddles up the code and template each time I have to remember to match container markup in two places or do something counter-intuitive.

+1 for subviews container functioning as a container instead of getting replaced by subview el.

[wherget]

Just to give this another bump: I was confused by the difference as well, but the confusion is not what irks me most; it's the double maintenance in the view and template. I would therefore also prefer appending to a container (+1 for #132).
While we're at it, shouldn't renderSubview then just be called appendSubview, just to further minimise confusion?

[pgilad]

I would add an append flag that signals we want to append the subview instead of having it replace the element. This was we maintain backwards compat.

[wherget]

I'm not really a fan of flags, though I will acknowledge that backwards compatibility is much easier to achieve using such a flag. However, it doesn't really help the confusion arising from (the default behaviour of) the subviews hash doing different things than renderSubview.