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

RFC: Leaving specified views intact during state transitions #562

Closed
MrOrz opened this issue Nov 6, 2013 · 46 comments
Closed

RFC: Leaving specified views intact during state transitions #562

MrOrz opened this issue Nov 6, 2013 · 46 comments

Comments

@MrOrz
Copy link

MrOrz commented Nov 6, 2013

I would like to send a pull request proposing a solution to #63 and #317, with two lines of code modification.

MrOrz/ui-router@angular-ui:master...patch-1

Summary of Changes

The template string of a state can be set to =. When entering such state, the named views with its template set to = will remain intact. Neither the scope nor the DOM will be destructed.

When = is introduced, implementing modal dialogs with ui-router can be straight-forward. I humbly provide a few motivating examples in the following sections. For each example, click "Edit in JSBin" button on the upper right corner to view its source.


Usage Example

// A page that lists all the items.
// Clicking the items will show the details in a modal dialog.
//
$stateProvider.state('index', {
  url: '/',
  templateUrl: 'index.html'
});

// The detail pages. 
// Populates "modal" view and keeps the main view.
//
$stateProvider.state('detail', {
  url: '/detail/:id',
  views: {
    '': {
      template: '='
    },
    modal: {
      templateUrl: 'modal.html',
      controller: modalCtrl
    }
  }
});

HTML Setup: Two ui-views, side-by-side.

<div ui-view></div>
<div ui-view="modal"></div>

Proof of Concept

http://jsbin.com/AYoKuHe/latest#/
Each "detail page" can be accessed individually via URLs. All the states are correctly managed by ui-router.

Notice that when the modal opens, the underlying index is still visible.

This is achieved by setting the template of main ui-view to = while in the "detail" state.

When the detail page is accessed directly via URL, the underlying page is blank.
http://jsbin.com/AYoKuHe/latest#/detail/4

Adding some simple trick and CSS magic, the detail pages can be shown outside of a modal when their URLs are directly accessed.
http://jsbin.com/itOjaj/latest#/detail/2

Notice the detail pages can still be viewed in modal after navigating to the index page. The behaviour is exactly the same as Facebook albums and Instagram colorboxes.

Integration with ui-modal

See it in action: http://jsbin.com/aFemAfe/latest
Individual detail pages are shown outside the modal: http://jsbin.com/aFemAfe/latest

The trick here is to move ui-view="modal" around. Kind of stupid, but it is somewhat better than fighting bugs caused by preventing state changes.

@timkindberg
Copy link
Contributor

Wow this is pretty bad ass... I'm trying to find holes in it though. I'll let you know.

@timkindberg
Copy link
Contributor

  1. So if a much deeper nest state wanted to set a top level view in the same manner would you need to set every other possible ancestor view to '='?
  2. Also not digging using '=' so much, we already have ^ and @, I think we should be more explicit. I would prefer a special string like '$retain' or a config property retain: true. Not saying 'retain' has to be the word but you get what I mean.

@nateabele
Copy link
Contributor

Certainly looks interesting. After a cursory review, the code looks okay, but one thing I noticed is that if you refresh, you lose the modal overlay. Did I miss a part in the description where this was intentional?

@deedubs
Copy link

deedubs commented Nov 6, 2013

Could we not make changing a named view opt-in? So you don't need '=' or retain, just don't specify a change?

Continuing with the above example

$stateProvider.state('detail', {
  url: '/detail/:id',
  views: {
    modal: {
      templateUrl: 'modal.html',
      controller: modalCtrl
    }
  }
});

I may be missing something obvious as I'm pretty new to ui-router...

@timkindberg
Copy link
Contributor

@deedubs that makes too much sense. But I haven't thought deeply about the implications. Seems like it would make for good usability from an API perspective too! I like it.

@Cinamonas
Copy link

This is a very welcome addition! +1

@jonrimmer
Copy link

I don't see how this would solve #63, as the tabs use-case requires that you keep the view/controller around when the ui-view it's attached to changes, not when an unrelated named view does. The best option for #63 remains some kind of caching capability on ui-view, IMO.

This is a pretty neat idea though, and would very useful for modals, other overlays, and things like sign-in dropdowns. Although for those, I would still prefer the ability to create additional state machines that could be independently serialized to and from the URL.

@RBLU
Copy link

RBLU commented Nov 7, 2013

Really like this and tried in our current app, works nicely, but I had to change your locals-comparison because it broke most state transitioning in our app (angular.equals ignores all properties starting with $ so it ignored many state changes) (using 1.2rc3)

if (locals === viewLocals || /* fast-check the obj references */
                  (locals && viewLocals && /* deep-comparing essential elements */
                      angular.equals(locals, viewLocals) &&
                      angular.equals(locals.$stateParams, viewLocals.$stateParams)) ) return;

I made it working like this:

if (locals === viewLocals || /* fast-check the obj references */
                  (locals && viewLocals && /* deep-comparing essential elements */
                      angular.equals(locals.$$state, viewLocals.$$state) &&
                      angular.equals(locals.$stateParams, viewLocals.$stateParams)) ) return;

@Cinamonas
Copy link

@RBLU That's exactly what I was about to report :) Relevant line in angular.equals source.

@RBLU
Copy link

RBLU commented Nov 7, 2013

made it working like this:

if (locals === viewLocals || /* fast-check the obj references /
(locals && viewLocals && /
deep-comparing essential elements */
angular.equals(locals.$$state, viewLocals.$$state) &&
angular.equals(locals.$stateParams, viewLocals.$stateParams)) ) return;

well, this quick fix is obviously not good enough. It needs both lines, the one I added and the one I replaced, otherwise the state is not updated when the locals ('resolves') change. So, better is:

if (locals === viewLocals || /* fast-check the obj references */
                  (locals && viewLocals && /* deep-comparing essential elements */
                      angular.equals(locals, viewLocals) &&
                      angular.equals(locals.$$state, viewLocals.$$state) &&
                      angular.equals(locals.$stateParams, viewLocals.$stateParams)) ) return;

@jeme
Copy link
Contributor

jeme commented Nov 7, 2013

@jonrimmer the templates are already cached, not as a detached DOM though, but keeping controllers around may have side effects, so it's more sane if that doesn't happen as part of a router solution IMO... (Basically my advice is to not make components with a router)

A tab view should instead be a component (directive + needed services) in it self... then just switch to the right tab based on a routing parameter... It's actually not that difficult to achieve... Doing this in an explicit way rather than weaving it into the router means people can be more aware of the side effects it may have...

A Very basic solution for a single tab view can be done in 21 lines of javascript + some html... As I demonstrated in #63 ... that just shows how simple it is... making it the right way (directive+services) would require a bit more work... but really...

From here on out it makes more sense to look towards: http://angular-ui.github.io/bootstrap/#/tabs for how to combine ui-router + ui-bootstraps tabs to achieve this (in your own solution)

That is just my point of view though...

@MrOrz
Copy link
Author

MrOrz commented Nov 7, 2013

I collected some modal related issues, which are listed below. These issues comes from different use cases and has different requirements. And I also found that my proposal did not resolve #63 :P

I hope the list give us insights on where we will be heading for, as well as to understand how far we can get with the simple modification proposed in my original post.


#475 : Parallel state handing

We have view A and view B, and we wish to change one view without touching the other.
retain or = would work for this case. However, if there are too many states in A and B, we will have to add retain in each and every state.

By making named-view refresh opt-in, which is proposed by @deedub, the state setup would be much cleaner and more intuitive. @deedub 's idea fits perfectly into this scenario.


However, the retain trick would not be sufficient in solving the following issues.

#490 : is there a way to handle multiple states?

As the previous issue, we have two views that want to work separately. However, this time we need to encode all the currently active states in the views into the URL, so that the views can be rendered correctly when the user visits the page directly via URL. #123, which is similar, requires remembering the page behind the modal dialog.

#63 : Optional support for keeping a view around

This issue differs from my issue because they want to preserve the compiled DOM, scopes and the controller even after the ui-view is populated with another content.

#160 discusses a much more general case, which can achieve complex structures like widgets and portlets.


As for my modification to $anchorScroll, I found that its existence was already challenged in #97 XD

In fact I removed $anchorScroll merely because it will scroll my page to the top when I quit the modal dialog.

@MrOrz
Copy link
Author

MrOrz commented Nov 7, 2013

@nateabele The modal overlay is deliberately removed when the detail page URL is accessed directly.
This is to mimic the behavior of Facebook albums and Pinterest overlays.

For instance, the page http://www.pinterest.com/merblood/if-i-do/ is a list of images. When an image is clicked, a modal dialog with much detail shows up, and the URL is changed to something like http://www.pinterest.com/pin/5348093281506292/.

When we directly access http://www.pinterest.com/pin/5348093281506292/ , the detail page is shown directly, without the modal overlay.

@MrOrz
Copy link
Author

MrOrz commented Nov 7, 2013

@RBLU @Cinamonas Wow I didn't know that before :P. It seems like I have overestimated what angular.equals can do. Thanks for pointing that out and the neat patch!

@jonrimmer
Copy link

@jeme We're probably getting a little off-topic here. I understand your point of view, and the code you provided is great, but as I think about it, my view is that caching goes beyond the use-case of tabs, to a broader use-case of managing the life-cycle of views and controllers.

In a rich desktop-like application, there can be a non-trivial cost to instantiating a controller, resolving its dependencies, and rendering a complex view into the DOM. Imagine an interactive dashboard, with lots data retrieved via ajax and displayed with D3 SVG charts that can be manipulated and filtered by the user. I will likely want to let the user switch to another part of the application, do some work, then come back to the dashboard and have it the same as they left it, but at present, everything will have been destroyed and need to be reinstantiated when they navigate back.

The usual advice is, "store everything in services", but this seems to me like a hack, because much of what you're storing in the services is not reusable data, but the state of the UI (such as filters, selected dropbox values, etc.). In rich UIs, the controller acts as a view model: It holds state related to the UI that is ancillary to the actual business data you want to display. When you store UI state in a service, what you're really doing is recreating your controller as a non-reusable, single-purpose service, just to take advantage of the service life-cycle, and that is an anti-pattern IMO.

The tabs use-case is just the simplest manifestation of a need to have parts of your UI whose state persists even when they are not being rendered. Yes, it's possible to do this by moving things outside of the view hierarchy and using ng-show, but I think the router should be the place where I configure the navigation hierarchy, templates, controllers, and resolves, and to that end, it should support more complex life-cycles for views and controllers.

@MrOrz
Copy link
Author

MrOrz commented Nov 7, 2013

Implemented @deedubs 's idea in a separate branch.

MrOrz/ui-router@angular-ui:master...patch-2

Example (ui-bootstrap integration):
http://jsbin.com/OkuZiTA/2/edit

@jeme
Copy link
Contributor

jeme commented Nov 7, 2013

@jonrimmer I have already debated against exactly what you mention in #63, the exact same example came up.

My whole point is that wrapping "Components" into views and control their state that way is an anti-pattern in angular to me... Angular has directives for a reason, they are extremely powerful, and i know they are a bit difficult to learn, so many people seem to dive into UI-Router to replace what they are actually their for... And that is not something I would like to help them do, because then I am really not helping them learning Angular, instead I feel like I am helping them to avoid Angular...

If you have a dashboard that needs to be "active" at all time, I would simply place it above or outside the scope of views... You can still control it's state through the UI-Router and it's controllers if that's needed... And it becomes more apparent that it's actually always their... Views are a special type of Directives, which have a template you wish to replace... the rest isn't views... it's components...

I have tried to lift an idea for a refresh opt-in for views for when source and target state is the same but parameters has changed, the controller and the view DOM would not be rebuild, but instead a refresh would somehow be communicated to the controller, as I think it could help in many of the scenarios people try to solve with independent states, or multiple active states.

If ill get the type ill try to leverage that with an example as well at some point...
As for getting off-topic... yes indeed... but we can just continue in #63.

@timkindberg
Copy link
Contributor

@MrOrz great summary several posts up. Very helpful.

@MrOrz
Copy link
Author

MrOrz commented Nov 10, 2013

I found that the demo page for patch-2 branch contains a bug.

Steps to reproduce:

  1. Visit http://jsbin.com/OkuZiTA/2#/detail/2 directly.
  2. Click "Return to index"
  3. Scroll down to the very end, you will find the content of the detail/2 at the end of the page.

This is because in patch-2 the views are not cleared by default. Now we to tell ui-router to clean up the view.

The demo page can be fixed by modifying the state definitions to:

$stateProvider.state('index', {
  url: '/',
  views: {
    "":{
      templateUrl: 'index.html'          
    },
    modal:{
      template: "" 
    }
  }
});
$stateProvider.state('detail', {
  url: '/detail/:id',
  views: {
    modal: {
      templateUrl: 'modal.html',
      controller: modalCtrl
    }
  }
});

See the fix in action: http://jsbin.com/OkuZiTA/3/edit

The question is, when I am using the patch in my own project, in the detail page I have links to multiple states. This means that I have to duplicate the "modal view cleanup" settings to these states. Is there any way to balance between the retain solution and the opt-in solution?

@smoke
Copy link

smoke commented Nov 25, 2013

This is great work and I believe this should be further integrated into the mainstream.

Here is one big note from me - the variant with explicit view behavior should be implemented e.g. template: '=' as this way developers get full control over the states.

The variant to not destroy views by default leads to big issues as there is no way to destroy them.

Thanks and great work again!

@mravey
Copy link

mravey commented Nov 28, 2013

I too hope that this feature make it into the mainstream. The solution with '=' or 'retain' seems better for me. I'm new to ui-router so I don't know if it's possible, be couldn't your first example be even more simplified to this for the detail state :

$stateProvider.state('detail', {
  url: '/detail/:id',
  views: {
    '': '=',
    modal: {
      templateUrl: 'modal.html',
      controller: modalCtrl
    }
  }
});

@MrOrz
Copy link
Author

MrOrz commented Nov 30, 2013

I'm glad to see people wanting this feature :)

I am planing to send a pull request from the patch-1 (the one using '=') branch. There is just a few things I'd like to figure out first.

  1. Which one is more preferable, template: '$retain' or template: '=' or retain: true ?
  2. Do I need to include test cases in the pull request? I'm new to test frameworks. Maybe I need some directions to get started.

@matthieu-ravey the simplified syntax looks great! 👍 It leads to further modification to other source files though.

@timkindberg
Copy link
Contributor

I vote retain: true

@nateabele
Copy link
Contributor

This still very much smacks of solving the wrong problem to me. State machine operations should be idempotent.

@chinchang
Copy link

👍 for retain: true

@nateabele State machine operations should be indempotent, alright. But how is this solving the wrong problem? Its a very common requirement to have a state affect only a certain view. Do you have some alternate method we could use to mimic the behavior of this PR?

@mravey
Copy link

mravey commented Dec 13, 2013

I also vote for retain: true !

@MrOrz
Copy link
Author

MrOrz commented Dec 13, 2013

@chinchang Maybe nateabele says that because separating routes and states as discussed in #160 is the ultimate solution addressing the root problem. Although it made its way into the milestone, it seems that the discussion has not converge yet. Since I cannot come up with an even better solution, I am currently using my own fork to keep rolling :P.

@underwms
Copy link

I vote for retain: true
This would solve so many issues I'm running into at the moment.

@chinchang
Copy link

@MrOrz Can you please help in rebasing your patch with the current/lastest master? The 1.2 animation fix has been merged and it has changed the updateView function quite significantly from what I can see.
Straightforward application of your patch is causing an issue where a view shows fine the first time a state is reached but renders blank the next time.
Appreciate your help.

@gigablox
Copy link

gigablox commented Jan 4, 2014

Would this solution allow you to update location path parameters without reloading the view?

The TLDR for the functionality I'm trying to achieve can be demonstrated here.

I have an collection of articles:

$stateProvider.state('articles', {
  url:'/articles/:id',
  templateUrl:'articles.tpl.html',
  controller:'articlesCtrl'
});

I'm using infinite scroll to display the articles in a feed.

  • As a user reaches the end of the feed we appendMore()
  • As a user scrolls into each article an updateLocation() function fires

The updateLocation() function is responsible for updating the location path to match the current article in the users viewport:

$scope.updateLocation = function(article){
  $state.go("articles.id",{id:article.seoUrl});
}

For example:

  1. I am currently viewing /articles/my-first-article.
  2. I reach the end of the article I scroll into the second firing updateLocation()
  3. The updateLocation()updates the location to /articles/my-second-article without reloading the view.

However changing or updating the $state reloads the templateUrl bounded to the controller which means the view will reload and jump to the top of the screen.

How do I update or replace the location path parameters without reloading the view?

@MrOrz
Copy link
Author

MrOrz commented Jan 5, 2014

@gigablox The function you are trying to achieve is impressive. It is exciting to see how scroll-spy-like interaction can be used to augment online-magazine reading experience. I have not thought of that before! Thanks for your sharing.

I am afraid that my solution cannot help you with such functionality. In your case the template for the state articles should be set to '=' to keep the view from being refreshed. However, the state should also be provided with a template for the very first article to render, which is currently not possible.

The template: '=' or retain: true solution targets for different ui-views, which fall short of your need on the same ui-view.

@MrOrz
Copy link
Author

MrOrz commented Jan 5, 2014

@chinchang Thanks for reminding me about the change!

The modal view is cleared up when leaving the model state (which is 'detail' in the example on the first post.)
but it is not rendered again when re-entering the modal state with identical parameters.

I removed the code doing deep-comparison between local and viewLocal and it worked again.

Actually I forgot why I was doing deep comparison 2 months ago...... 😕

Here is the branch now: (Beware, I did a force-update on the branch!)
https://github.com/MrOrz/ui-router/compare/patch-1

Here is a working example:
http://jsbin.com/eWayOyiz/latest#/

Also note that I've added autoscroll="false" in the two elements with ui-view directive in HTML, to prevent scrolling on modal open & close.

@chinchang
Copy link

@MrOrz Deep comparison is required as the references (locals and viewLocals) are not same. I found the reason though...it happens because viewLocals is not being set to locals in the block which runs when locals is empty. So this seems to work now:

function updateView(shouldAnimate) {
          var locals = $state.$current && $state.$current.locals[name];

          if (isDefault) {
            isDefault = false;
            element.replaceWith(anchor);
          }
          if (!locals) {
            cleanupLastView();
            currentEl = element.clone();
            currentEl.html(initial);
            renderer(false).enter(currentEl, parentEl, anchor);

            currentScope = $scope.$new();
            $compile(currentEl.contents())(currentScope);

            //------------------------- ADDED THIS -------------------------
            // Reset viewLocals to locals. Otherwise on next update when `locals`
            // is populated, it gives true on deep equality check with `viewLocals`.
            // This is basically required because of the additional deep-equality check
            // in the view retain code below [1].
            viewLocals = locals;

            return;
          }

          // [1] nothing to do if view to be rendered is same as last one.
          if (locals === viewLocals ||
            (locals && viewLocals && /* deep-comparing essential elements */
              angular.equals(locals, viewLocals) &&
              angular.equals(locals.$$state, viewLocals.$$state) &&
              angular.equals(locals.$stateParams, viewLocals.$stateParams)) ) return;

          // Preserving current view and scope if current $template is '='.
          if (locals && locals.$template === '=') return;

          cleanupLastView();

@MrOrz
Copy link
Author

MrOrz commented Jan 5, 2014

@chinchang Thanks for digging into the cause and the solution you provided!

I am thinking that, if merely checking the references is adequate in the original implementation of updateView (before my fork) , why should we do the deep comparison and further reset the viewLocals references?

@MrOrz
Copy link
Author

MrOrz commented Jan 5, 2014

Maybe the question "Is it proper to update viewLocals here" is deeply related to the intended use of the viewLocals variable. I am not sure what "viewLocals" actually means......

@chinchang
Copy link

From what I get, viewLocals is kind of misleading. It should be lastLocals instead as it simply backups the value of locals whenever a view is actually updated so that on next update we can return early if nothing has changed between the two.

I checked the original reference equality check and found that even that isn't true :P
Which is correct as per the intention of ui-router i.e. to update every view described in a state. It is our need of retaining a state that needs deep comparison.

That makes me wonder, which is that situation when the two references actually are same. May be just a precaution to not re-render views if transitioned state is same as current one.

(Just in case if you have not noticed, your main view renders every time even in modal state with that deep-comparison removed)

@MrOrz
Copy link
Author

MrOrz commented Jan 5, 2014

Yes the main view re-renders every time when leaving modal state without deep comparison.

I didn't notice that. Stupid me 😐

@chinchang Thank you so much for pointing that out!

The branch has been updated : https://github.com/MrOrz/ui-router/compare/patch-1

Now I includes the render timestamp in the example. http://jsbin.com/EpUDELU/4#/

@chinchang
Copy link

Cool. No probz 👍

@gigablox
Copy link

gigablox commented Jan 6, 2014

@MrOrz Thanks for your ideas. Over the weekend I was able to find a little hack to achieve this functionality.

It became obvious that the only thing I really wanted to change was the $location because our updateLocation() and appendMore() function are in the same controller and we need those things to continue doing their jobs.

So instead of messing with $state lets mess with native browser functions like history.replace().

$scope.updateLocation = function(article){
  $location.path('/articles/'+article.seoUrl);
  history.replaceState({}, '/articles/'+article.seoUrl, '/articles/'+article.seoUrl);
}

$rootScope.$on('$routeChangeStart', function(event){ 
    event.preventDefault(); 
});
  • First invoke $location.path() on the target URL.
  • Then intercept the request and prevent any action from taking place.

I am doing this first because of a bug in the AngularJS $location service. Basically you are making sure $location is aware of the history.replaceState() that is about to happen. By executing $location.path() you can keep the URL change in sync to prevent the error - and by event.preventDefault() you can prevent the controllers or views from reloading.

  • Finally we can use history.replace() to update the URL without affecting the current $state or cause conflicts with the $location service URL on change event listeners.

@MrOrz
Copy link
Author

MrOrz commented Jan 6, 2014

@gigablox glad to hear that there is a work-around available 👍

@timkindberg
Copy link
Contributor

@gigablox awesome. Could you please close your other issue that was asking the same question and don't forget to share you solution on your StackOverflow, that will be a great resource for others!

@chungwong
Copy link

@gigablox thanks for your idea. For some reasons I couldn't get your idea working. It definitely stopped the state transition, but the URL didn't get preserved as it immediately changed back to the original URL from the new URL.
here is a fork of UI router example. if you rapidly click the link "Go to route2", the URL would keep flashing between route1 and route2.
http://plnkr.co/edit/m20wYX?p=preview

I have tested it in Chrome and FIrefox( for firefox i have to open firebug to see the URL flashing).

Do you encounter this weird behaviour?

@gigablox
Copy link

@chungwong Yea I ended up going about it completely the wrong way. I ran into that problem later on not realizing it and was pretty bummed out haha.

In the end I was able to use purely $state without a $location hack.

For your use case I would recommend doing something like:

$stateProvider.state('ctrl', {
  url:'/stuff',
  template:'<ui-view autoscroll="autoscroll"/>'
});

$scope.autoscroll = false;

@chungwong
Copy link

@gigablox thanks for your reply. How does autoscroll solve this problem? The fork is basically attempting to do what your demo http://qz.com/ does. I don't really get how autoscroll solve this nasty situation.
Thanks

@tomsaffell
Copy link

Is this RFC making any progress? Or has an alternative solution been implemented elsewhere? Seems like a good idea to me :)

@MrOrz
Copy link
Author

MrOrz commented Aug 2, 2014

@tomsaffell Sorry for the delay; the discussion continued at another RFC here: #894 (comment)

@christopherthielen has provided a great implementation of sticky "states", which is extracted to a separate angular module ui-router-extras. It worked very well in the Pinterest-like modal scenario, and I had a great time using ui-router-extras.

I guess this issue can be closed now, preferring ui-router-extras.

@MrOrz MrOrz closed this as completed Aug 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests