fix(uiView): Refactoring uiView directive to copy ngView logic

[meenie]

Use the linking function element instead of the template element when setting the parent.

Fixes issue #857

[nateabele]

The fix looks good, thanks for putting it together. Two things before I can merge it: (1) unit test (2) the commit message needs to be reformatted to comply with the contributor guidelines

I'm happy to do it myself when I have some free time (probably not for a few days), or you can take care of it and update the PR. Totally up to you.

[meenie]

Hi Nate, I'll fix it up later today.

[nateabele]

Awesome, thanks.

[meenie]

@nateabele - I've changed the commit message and put in a regression test. Also, my editor has added a new line to the end of viewDirectiveSpec.js. It should probably be there, since it's common practice to do so. Let me know if you'd like me to take it out.

[jonrimmer]

This still does not work. It fixes the lookup of the state information, but views still fail to render when the state of the ng-if has been flipped from on to off and then back to on. You can verify this by extending your unit test to flip the boolean's state for a second time.

The problem seems to be that that the view directive uses a comment anchor as the insertion point for the views, but it inserts this into the template element. If the ng-if has re-rendered its contents, the template element containing the anchor is no longer attached to anything.

I couldn't really say how to fix this. I don't understand why the directive is using a comment anchor in the first place. Does anyone actually understand the design of this directive, or is it just hacks upon hacks? The fact that this has been broken for months suggests not.

[meenie]

@jonrimmer - I don't have the time at the moment to look more into this as what I've done actually fixes the issues I'm having. Huge deadline at work right now. I'll come back to this later and see if I can really fix this issue.

[nateabele]

I couldn't really say how to fix this. I don't understand why the directive is using a comment anchor in the first place.

As far as I understand it, this is necessary to properly handle animation. If anyone is interested in porting over the implementation for ngView, let me know.

[meenie]

I'm going to have another stab at this. As for it using a HTML comment for animation, that shouldn't be the case.

[meenie]

@nateabele - I've refactored uiView to be more like ngView. This fixes the issue I was having with ngIf. It also fixes the issue that @jonrimmer was having when toggling the ngIf boolean back and forth.

Changes to Tests
I've updated the tests so they all pass. I've also updated my new ngIf test to be a bit more extensive as well. I needed to skip this test for AngularJS 1.0.8 since ngIf does not exist in that version.

In the previous version of the tests, the first $animation event on the queue was a leave. In this refactored version of uiView, the first event is now enter. I feel that that is the correct order. If anyone else would like to comment on this, please feel free.

Know Issues
I've found an issue when using AngularJS 1.1.5 and putting the ngIf on the same element as the uiView. When the ngIf is given a falsey value, it does not remove the element from the DOM. If you place the ngIf on a parent, it works fine. It would be great if I could get a second set of eyes on this to see what I'm missing.

Also pinging @christopherthielen as they were having the same issues.

[meenie]

This should also fix #552

[christopherthielen]

uiView was refactored to NOT transclude in commit a402415 2013-12-27. This commit switches back to transclude (which matches what ngView is doing). @nateabele, why switch from transclude back then?

@meenie Assuming your changes don't break anything, it looks nice to me. This code is significantly easier to follow!

[meenie]

@christopherthielen - Thanks :). It's a lot more "Angular" and less "fighting the man" I think.

[meenie]

I've done more refactoring and uiView now supports ngRepeat, ngClass, and ngIf. The ui-view and name attributes now work with interpolation (<ui-view name="{{myVar}}">Loading...</ui-view>. Feedback would be much appreciated.

Regarding Tests
I noticed that all assertions were wrapped in if ($animate) which means none of them were running for AngularJS 1.0.8. I've removed ngAnimate and replaced it with ngAnimateMock so I can use $animate.triggerCallbacks(). This also means I needed to update the base version AngularJS from 2.1.4 to 2.1.14 to get the $animate.triggerCallbacks() feature. What needs to happen now is a section in the tests where we do include ngAnimate and test to make sure that animations are properly working (adding the right classes, switching out content, etc). I didn't think the current version of the tests should be doing any animation testing as it's too much going on inside each describe() functions. There also needs to be tests written for ngRepeat and ngClass. I will most likely be tackling both of these this weekend.

Known Issues

• AngularJS 1.0.8
  • ngRepeat - This feature does not work in Angular 1.0.8. You need to use attr.$observe() to get them to interpolate and I can't do that because it causes the second uiView directive to be asynchronous from the first uiView directive.
• AngularJS 1.1.5
  • ngRepeat - This feature somewhat works, but seems to reverse the order that the uiView directives are rendered. No effing clue why that is the case. I'm not fussed because ngRepeat is probably going to be the least used feature of uiView hah.
  • ngIf - You cannot place it directly on a uiView directive, it has be on an element that is at least one DOM level above.

Pinging @nateabele @timkindberg @christopherthielen

[nateabele]

Well shit man, that saves me a ton of work! Thanks! 🍻

I've gotta step out, but I'll review the code as soon as I get back.

[meenie]

For a preview of the new features, I have a shitty example right here :) - http://embed.plnkr.co/Dy5NHsq5eyaJPOJirsP6/preview

You can show/hide using ngIf, turn on/off animations using ngClass, and see how ngRepeat works.

[timkindberg]

@meenie 😍

[nateabele]

@meenie You, sir, are to be commended the GitHub Medal of Honor, the highest honor in the land (...actually I just made that up).

Re-known issues... the 1.0.8 issue we can note in the FAQ. For 1.1.5, the ngRepeat issue I wouldn't worry about, as it's an unstable version anyway, and uiView and ngIf both require transclusion, so people shouldn't expect them to work on the same element anyway.

In summary, very well done sir. Your efforts are sincerely appreciated. If I can wrangle up some people to test this against IE, I will happily tag this as 0.2.9.

[meenie]

@nateabele @timkindberg - No worries! It was a great lesson on Directives :). There is a rabbit hole that you can get very far into... Luckily I came back out with only a few bumps and bruises haha.

Re IE....who uses that anyway 😉

[meenie]

BTW, transclude: 'element' is very cool. As long as the priorities are set right, you can transclude over many different directories and it should have no issues. With this new refactor, you can have ngIf, ngRepeat, and uiView all on the same element without issue (if not using 1.1.5) 😃

[gampleman]

Can someone confirm: does this fix #774?

[meenie]

@gampleman - yes - http://plnkr.co/edit/qhO16n0dvoJsXLtuVWAk?p=preview 😄

[marcghorayeb]

@meenie Nice PR

[meenie]

@marcghorayeb thanks 🍻!

[kumarharsh]

thanks a lot!

[jonrimmer]

Nice work! 👍

[timkindberg]

@meenie feel free to close any issues you find that this fixes. You rock so much dude.

[timkindberg]

I will happily tag this as 0.2.9.

@nateabele there is already a 0.2.9... I thought you added it.

[meenie]

@timkindberg - I'm not able to close issues.

[gabrielmaldi]

@meenie thanks a lot for this! I thought I wouldn't be able to count on ng-repeat working with ui-view anymore (#825) 😃

A question: is it expected that I get comments like the following before every ui-vew?

<!-- uiView: undefined -->

This happens even on ui-views with constant (i.e. not interpolated) names.

[meenie]

@gabrielmaldi - no worries :). I still think that using a ngRepeat is a symptom of a very complex design where it might be better fixed using nested views. It sounds like your application is very linear rather than tree structured.

As for the <!-- uiView: undefined --> comment tags. That happens when you use uiView like <ui-view></ui-view>. If you were to do <div ui-view></div> then the comment would be <!-- uiView: -->. And if you did <div ui-view="test"></div>, it would be <!-- uiView: test -->.

I'm not sure if this is a issue yet though....

[timkindberg]

@timkindberg - I'm not able to close issues.

You are part of the team now, so close issues until your heart is content.

[meenie]

@timkindberg Thanks :). Will do!

[meenie]

@nateabele - I fired up my windows machine and tested in Windows 8.1 using IE 11 and then using the developer tools to emulate 10, 9, and 8. Everything is working fine.

[nateabele]

@meenie Sounds good. I'll tag a release tomorrow, and look at getting Sauce Labs set up so we can have CI against multiple browsers. Thanks again for your work on this.

[christopherthielen]

#1324