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

Make ui-view retain inner content as "no view" template #142

Closed
adambabik opened this issue May 23, 2013 · 29 comments
Closed

Make ui-view retain inner content as "no view" template #142

adambabik opened this issue May 23, 2013 · 29 comments

Comments

@adambabik
Copy link
Contributor

Hi,

I've been testing $stateProvider as it's really great replacement for crippled $routeProvider and found interesting behaviour. When the element with ui-view directive is not empty, the first load of the template doesn't replace its content, but works like append. However, the next load replace the whole content of this element.

You can check a live demo here.

Important: This problem occurs only using AngularJS in version 1.1.4. The current stable version works as expected.

@timkindberg
Copy link
Contributor

Does the problem occur with ng-view as well?

@jeme
Copy link
Contributor

jeme commented May 24, 2013

I think I saw an Issue like this in the Angular Core framework for the animator... see if I can dig it out at some point and link it.

Have you tried with 1.1.5 by any chance?

@adambabik
Copy link
Contributor Author

@timkindberg Do you mean ng-view from ui.comapt? I checked it and it's working properly. Here is a demo. Strange as both directives use the same function $ViewDirective.

@jeme Oh, I didn't know that the new version came out. I checked with 1.1.5 and the same problem exists. However, with 1.0.7 (and 1.0.6) it's just fine.

@adambabik
Copy link
Contributor Author

I dug dipper and the animator is involved indeed!

When rendering the template for the first time, $animator.leave is not called, because there is no existing viewScope. Instead, the $animator.enter is called, which is doing something like this: parent.append(element). So it actually appends the template. When the view tries to render the template again, the current viewScope is available, then $animator.leave is called and it's doing element.remove(). In fact, the element contains the whole content - existing and the append one. So later, $animator.enter appends the template to the completely empty parent element.

Why the example with ng-view is working correctly? I might be wrong but it seems like the original ng-view is called and it uses animate.leave always when updating the content, even for the first time. So it clears the content and then append the template.

@jeme
Copy link
Contributor

jeme commented May 24, 2013

Indeed, your right... should properly move these lines https://github.com/angular-ui/ui-router/blob/master/src/viewDirective.js#L33-L34 out of the if(viewScope) block.

@adambabik
Copy link
Contributor Author

Yes, I have just tested the same solution and it's working fine. Do you want me to make a pull request or you'll handle this?

@jeme
Copy link
Contributor

jeme commented May 24, 2013

@dreame4 Sure, I am at work and won't get time during the day later... Also I don't know if there is other implications of moving it out (It doesn't seem so to me)... So with a PR we can have a more formal evaluation of it.

adambabik added a commit to adambabik/ui-router that referenced this issue May 24, 2013
@ksperling
Copy link
Contributor

The change looks fine to me, assuming that the animator is smart enough to not do anything if there is no content to animate.

A related question, should ui-view copy aside the initial content, and put it back if/when the view becomes unassigned? Effectively it would act as the 'this view is currently empty' template.

@timkindberg
Copy link
Contributor

@ksperling said:

A related question, should ui-view copy aside the initial content, and put it back if/when the view becomes unassigned? Effectively it would act as the 'this view is currently empty' template.

That's a cool idea! Would cut down on a templateUrl load.

@nateabele
Copy link
Contributor

Agreed, that would make ui-view way more useful.

@adambabik
Copy link
Contributor Author

@ksperling I have tested several cases, including this, and the animator works just fine.

A related question, should ui-view copy aside the initial content, and put it back if/when the view becomes unassigned? Effectively it would act as the 'this view is currently empty' template.

Totally agree!

@nateabele
Copy link
Contributor

@dreame4 Can you modify your patch and squash the commits?

@adambabik
Copy link
Contributor Author

@nateabele Yes, I'll work on this today.

adambabik added a commit to adambabik/ui-router that referenced this issue May 24, 2013
adambabik added a commit to adambabik/ui-router that referenced this issue May 24, 2013
adambabik added a commit to adambabik/ui-router that referenced this issue May 24, 2013
@adambabik
Copy link
Contributor Author

@nateabele I made it a little bit messy... Sorry about that, but for the first time I was trying to squash already pushed commits :/ Anyway, now my pull request points at the proper commit so you can look at this. The code that puts back the initial view is really short. I tested it using the sample app from the repo and it worked fine.

nateabele added a commit that referenced this issue May 24, 2013
Fixed issues #142 (removing ui-view content at the first load)
@nateabele
Copy link
Contributor

Fixed in #144.

@ksperling
Copy link
Contributor

Hm, I think this is oversimplified -- does this work correctly if the initial content contains directives, especially after the content gets put back?

I think we'll need to start having some unit tests for this sort of stuff.

@ksperling ksperling reopened this May 24, 2013
@adambabik
Copy link
Contributor Author

@ksperling Unfortunately, it does not. Currently, it's fine for simple "no content here" text and plain HTML but that's all. I agree it should work with all (or most) features provided by Angular.

I think I can try to implement it, so the initial content could handle directives, filters and others. And yes, unit tests are a must in this case.

@ksperling
Copy link
Contributor

I've been looking at https://github.com/angular-ui/angular-ui/blob/master/modules/directives/if/if.js

Something similar should work here, even though I think it might need different transclude options in terms of interactions with other directives on the same element.

@adambabik
Copy link
Contributor Author

@ksperling Thanks for the source. It was a great inspiration. I managed to create ui-view directive which handles the initial view including parsing it, setting up other directives etc.

Additionally, I created some tests for $ViewDirective. It covers the basic usage plus the initial view case. You can find it here: https://github.com/dreame4/ui-router/tree/initial-view. I'm not very experienced at writing AngularJS code so, I would appreciate it if you (or anyone else) could look at the code and give me some feedback. Then I'll clean up the codebase and send a pull request.

@timkindberg
Copy link
Contributor

Let's not forget about this one, maybe just do a pull request and if it needs cleaning up, @ksperling will reject it with comments.

@ksperling
Copy link
Contributor

@dreame4 this looks great, and good to see some tests too! please go ahead with the tidy up/PR.

@adambabik
Copy link
Contributor Author

@ksperling thanks for the review! I've cleaned up the codebase and made a pull request.

@nateabele
Copy link
Contributor

With the merging of #171, I'm guessing we can close this?

@adambabik
Copy link
Contributor Author

Totally. Thanks for reminding.

@morgs32
Copy link

morgs32 commented Jul 2, 2013

Is this fixed or not? I've updated your original plunkr here and nothing's changed. http://plnkr.co/edit/IG5gek?p=preview

@nateabele
Copy link
Contributor

@morgs32 From the build you linked to (http://angular-ui.github.io/ui-router/build/angular-ui-router.js):

* @version v0.0.2-dev-2013-05-25

@morgs32
Copy link

morgs32 commented Jul 2, 2013

Looking at my comment - it wasn't humble enough. I'm noob enough to ask how do I go about implementing this fix. Do I download the src folder and include each js file in my html?

@nateabele
Copy link
Contributor

@morgs32 No worries. I suggest building it manually from source. Just clone the repository and follow the instructions here: https://github.com/angular-ui/ui-router#developing

It will spit out minified and non-minified versions in the build/ directory.

@morgs32
Copy link

morgs32 commented Jul 3, 2013

First time on grunt - that was awesome. Thanks for the help.

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

6 participants