Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

replace:false in directive doesn't "append" as documented #2235

Closed
jokeyrhyme opened this issue Mar 27, 2013 · 28 comments
Closed

replace:false in directive doesn't "append" as documented #2235

jokeyrhyme opened this issue Mar 27, 2013 · 28 comments

Comments

@jokeyrhyme
Copy link
Contributor

From the official documentation http://docs.angularjs.org/guide/directive (search for replace -):

replace - if set to true then the template will replace the current element, rather than append the template to the element.

I am using replace:false in combination with templateUrl:.... The target element in question already contains some content generated by a server-side script, and I merely wish to add to it with a directive. The documentation suggests that replace:false will cause the template content to be appended to the target element.

Instead, the template content completely replaces the content of the target element. The culprit lines of code are here:
https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L695
https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L1012
Notice that angular.element.html() is being used here instead of angular.element.append().

Should the documentation be altered to match the implementation? Or the reverse?

Also, is there a temporary work around I can employ to get my template data playing nicely with existing content as desired?

Cheers.

PS. I am using jQuery 1.7.2 with Angular JS 1.0.5.

@JakobJingleheimer
Copy link

Nudge

I also noticed this problem. I would really prefer it actually do what the docs say it does (rather than changing the docs to reflect the current behaviour).

Tested on AngularJS 1.0.4 & 1.0.6

@JakobJingleheimer
Copy link

Additionally, https://github.com/angular/angular.js/blob/master/src/ng/compile.js#L990 would have to be removed (if the behaviour of replace:false is to be to append).

@m-rouget
Copy link

m-rouget commented Jun 3, 2013

I encountered the same issue.
By reading the documentation that says: replace - if set to true then the template will replace the current element, rather than append the template to the element.
... I though it appended the template to the element, after existing contents.
However, after I read this this explanation, I found that it appends the template to the element, but removes any existing content of the element.

If this is intended behavior, the documentation should be updated:
replace - if set to true then the template will replace the current element. If set to false then the template will be inserted as a child of the element, removing existing children of the element.

At the end, there should be a third option to replace, which insert the template as a child of the element, at the end of existing content.

@JakobJingleheimer
Copy link

Yes, I agree with @m-rouget: there should be 3 options: replace current element, replace current element's children, or append to current element (which would insert the template after the current elemen's last child). I'm not sure how to best convey that as a property tho. What I've thought of so far is

replace: {string:"element","children","append"}

I think it should all be on the same property to avoid confusion—as opposed to a "replace" property and an "append" property, and also because they are mutually exclusive.

Since that is likely to take (me?) some time, I suggest updating the docs first to reflect the current behaviour: it either replaces the current element or replaces its children.

@jokeyrhyme
Copy link
Contributor Author

What if there was a placeholder you could put in the template that would represent where to put existing children?

It's a bit more complicated, but you have full control over the result and you get "prepend" behaviour for free if you want it.

Example:

Current DOM structure:
< div id="target" >
< span id="existing" />
< div id="target">

Template:
new content is mixed in with the {{ currentChildren }} and we are all happy!

Result:
< div id="target">
new content is mixed in with the < span id="existing" /> and we are all happy!
< div id="target">

@JakobJingleheimer
Copy link

@jokeyrhyme I don't understand how that could happen. In JavaScript, you have 3 choices for setting the content of a DOM element: setting the element.innerHTML property (which replaces the content of element to whatever you set it equal), or using either the element.append() or the element.prepend() methods.

Also, there is ng-transclude which you can place in the DOM to tell it where to insert transcluded content:

<ul my-directive-that-replaces-content>
  <li>Foo</li>
  <li ng-transclude></li>
  <li>Bar</li>
</ul>

@jokeyrhyme
Copy link
Contributor Author

The append and prepend that you speak of are not in JavaScript, just in jQuery (or Angular's jQuery Lite). We aren't limited to using these methods.

Thanks for your ng-transclude example, I keep forgetting about that. Is there a way we can use the transclude option when defining a directive to achieve this (without needing to modify Angular) ?

@JakobJingleheimer
Copy link

@jokeyrhyme, yes that's true they're from jQuery/jqLite.

I think Angular will still need to be modified because you can't transclude from the same element multiple times. I haven't seen an explanation anywhere, but I'd guess they designed it that way assuming the current functionality of replace (so to avoid needless DOM manipulation).

@btford btford closed this as completed Aug 24, 2013
@btford
Copy link
Contributor

btford commented Aug 24, 2013

As part of our effort to clean out old issues, this issue is being automatically closed since it has been inactivite for over two months.

Please try the newest versions of Angular (1.0.8 and 1.2.0-rc.1), and if the issue persists, comment below so we can discuss it.

Thanks!

@james-lawrence
Copy link

Still exists in 1.2.0rc1

@chrisnicola
Copy link
Contributor

@btford yes still around and quite problematic. I'm trying to create a directive that adds some buttons around an input and it's placing them inside the input tag.

@7hi4g0
Copy link

7hi4g0 commented Sep 17, 2013

@btford I'm experiencing this bug also.
I'm using version 1.0.7 but I believe this bug is a vital one.
I would rather have append to be the default behaviour, even with template or templateUrl.

@chrisnicola
Copy link
Contributor

On this note it would be nice to have some flexibility on the behaviour here: append, prepend, insert, all seem like valid scenarios depending on the need. The alternative it to manipulate the DOM directly but that seems to unnecessarily limit the value of the template.

@btford btford reopened this Sep 18, 2013
@btford
Copy link
Contributor

btford commented Sep 18, 2013

I'm not convinced that each permutation of this behavior should be exposed as an option for directives since you can use a compile function to resolve the other cases, but I do agree that the docs and the behavior should coincide.

@jokeyrhyme
Copy link
Contributor Author

Here's a pattern for directives that I use now for cases like this:

var app = angular.module('app', ['ng']);

app.directive('myDirective', [
  '$compile',
  function ($compile) {
    return {
      replace: false,
      terminal: true,
      link: function ($scope, el$, attrs) {
        var template, new$;
        template = ''; // some HTML structure, maybe include the existing DOM children?
        new$ = angular.element(template);
        el$.replaceWith(new$); // can do anything you want instead of replace
        $compile(new$)($scope);
      }
    };
  }
]);

So it's not too difficult to work-around and get the exact behaviour you want, just a little inconvenient. I agree that the main concern now is that the documentation isn't precisely accurate.

@chrisnicola
Copy link
Contributor

@jokeyrhyme thanks for the workaround, that's neat. It's certainly ideal to have this baked in of course. Perhaps someone in this chain can find a bit of time for a pull-request?

@btford are you suggesting these are edge cases (I disagree) or that it's ok to have to expose developers to using $compile or a compile function for this (also disagree) or something else entirely?

@wprater
Copy link

wprater commented Sep 25, 2013

The appending has already occurred during the directives compile process, so you need to do a workaround as stated by @jokeyrhyme

@wprater
Copy link

wprater commented Sep 25, 2013

@jokeyrhyme setting the terminal = true caused my other nested directives to not be rendered. I suppose if I were to include them in template they would have been compiled?

@jokeyrhyme
Copy link
Contributor Author

@wprater you have to explicitly call $compile on the new content once your done manipulating it. Without terminal = true, Angular.JS will automatically run $compile for you, but then you don't have any opportunity to mess with the DOM structure first.

@KODerFunk
Copy link

+1 for appending after if replace: false

@chrisnicola
Copy link
Contributor

This isn't really a solution, as stated before only being able to insert (or replace even) the contents of an element with a directive is very limiting and even impossible for certain elements. I hope someone will consider reviewing this. I feel the design of this could be better and it will continue to be confusing and problematic for developers using it.

The workaround using the $compile service is clunky, ugly and confusing. I would never use it by choice in my own codebase and consider it a hack.

@saem
Copy link

saem commented Oct 2, 2013

@btford I don't see how prepend, replace, append has significant downsides? Three permutations, which keeps the code declarative seems ideal.

I say this as a user and teacher of angular. Compile is a deep dive into angular, just to get what should be, in this case, shallow behaviour.

Directives being the bear that they are, a few conveniences would go a long way to alleviate those pains.

As an aside, I think this is a further delineation between "angular core" and "angular porcelain"; competing concerns between a spare core, and a API consumer convenience. To this end I'm glad to see further modularization.

@petebacondarwin
Copy link
Contributor

@saem - Moving to a declarative method for this is a good idea in the long run. We should look at it post-1.2 release as it is a large complex effort to keep track of the various DOM node changes through the async life-cycle of the compilation process.

@jokeyrhyme
Copy link
Contributor Author

This ticket originally posed two solutions: updated documentation or a change in behaviour.

Now that the documentation has been updated, it would probably be better to start a new issue for changing the behaviour (e.g. adding new options for append / prepend / insert / etc).

@7hi4g0
Copy link

7hi4g0 commented Oct 2, 2013

The change made on the docs made more clear what should happen, but they still describe the same thing as the old docs and the behaviour described is not happening. http://plnkr.co/edit/biQofl6UVAhilw5vg5k2?p=preview At this plunker you can see that the behaviour is as expected in the first outter element, but when I do a ngRepeat the behaviour is not the way you would imagine it to be (at least for me). 👯

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Previous version stated `replace:false` will append template to element.
Improve description to accurately state that template will _replace_ the
contents of the current element.

Closes angular#2235, angular#4166
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Previous version stated `replace:false` will append template to element.
Improve description to accurately state that template will _replace_ the
contents of the current element.

Closes angular#2235, angular#4166
@farolfo
Copy link

farolfo commented Jan 31, 2014

+1 for .append() instead of .html() if replace: false

@scyrizales
Copy link

someone resolve this? I really need this... I will go with the workaround in the meantime.

@JakobJingleheimer
Copy link

This feature will soon be removed. From the $compile docs:

replace ([DEPRECATED!], will be removed in next major release)

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

No branches or pull requests