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

feat(uiViewCache): outer most views can now be reused #2333

Closed
wants to merge 2 commits into from

Conversation

FDIM
Copy link

@FDIM FDIM commented Oct 25, 2015

This PR resolves #1800 and enables reusable / persistent views. When user navigates to a persistent state it will be exactly as it was before navigating away. This greatly improves user experience if you are targeting mobile apps or have complex elements in page.

By adding 'persistent:true' flag to a state config you indicate that it is reusable. Such states will only be initialized once and through the use of additional events you can control what happens when user revisit same state and control it's lifetime.

There are 2 additional events:

  • $viewRestored can be used to do partial updates to a view. E.g. change location of map pins based on different state.params, reload list view when search criteria changes, etc.).
  • $viewCached provides you a reset function that will clear cache of this view so that it would be reinitialized next time user navigates to this state.

and there are 2 quirks:

  • Only leaf nodes are supported. Behavior is undefined for views that has inner views.
  • Since DOM node is detached, scroll position is lost. Directive below can be used to overcome this issue. It might be ok to handle this automatically, but some input would be needed from $viewRestored event - this is the place where developer would know whether view is going to be refreshed or not.
(function (module) {
    var DATA_PROP = '_scrollTop';
    module.directive('persistentScroll', function ($timeout) {
        return {
            restrict: 'A',
            link: function (_scope, _element) {
                _scope.$on('$viewRestored', function () {
                    _element.scrollTop(_element.data(DATA_PROP)||0);
                });
                _element.data(DATA_PROP, 0);
                _element.on('scroll', function () {
                    $timeout(function () {
                        _element.data(DATA_PROP, _element.scrollTop());
                    });
                });
            }
        };
    });
    // The name of the module, followed by its dependencies (at the bottom to facilitate enclosure)
}(angular.module("app.persistent-scroll", [
])));

@nateabele nateabele modified the milestone: 0.2.16 Oct 26, 2015
@nateabele
Copy link
Contributor

Nice.

I wanna poke at it a bit to see if I can clean up the code that interacts with ngAnimate, and see if I can figure out a way to at least drop the leaf-nodes-only restriction. It's also gonna take a bit of workshopping to make it 1.0-compatible, since the view layer no longer directly interacts with $state.

@christopherthielen I think we should probably just drop 0.2.16 and then feature-freeze 0.2.x.

@FDIM
Copy link
Author

FDIM commented Oct 26, 2015

I didn't like animation code either, but I though it's best to not remove backwards compatibility (not for me to decide anyway) - Is angular 1.2 still relevant for this library?

Regarding leaf-nodes quirks/restriction, it might not be an issue at all if inner views are cleaned up/removed before parent when navigating to a new state. So that cached entry would only have placeholders for inner states. I looked into what goes wrong, but I couldn't figure out exact problem/pattern. No errors are logged to console, view just stays empty or looses interactivity.

Other than that, it might be convenient to be able to set persistent flag on per view basis (I can submit another PR at some point or push another commit, let me know if this is of interest to you).

Do you have any opinion about handling scroll offset? Should it be be taken care automatically or left for developers to maintain?

@nateabele
Copy link
Contributor

Do you have any opinion about handling scroll offset? Should it be be taken care automatically or left for developers to maintain?

It'd be nice if we could figure out a way to do the 'right' thing by default, but I don't think it's worth holding up the feature over.

@FDIM
Copy link
Author

FDIM commented Nov 1, 2015

Just an FYI, I've ran into a small issue with viewRestored event. As it is now it is $emitted from controller scope therefore child elements have no clue when it happens.
I am thinking of changing this logic so that event would be broadcasted from rootScope. Do you @nateabele have any concerns in doing so?

@nateabele
Copy link
Contributor

Hey, sorry for taking forever to come back on this. Hmm... how would listeners have context for what's happening (i.e. since most views are unnamed)?

@FDIM
Copy link
Author

FDIM commented Nov 30, 2015

That's a good question... I am not sure if anything else besides view name (more like path) makes sense. Anyway - I worked around the issue described above (couldn't find place in code), I just remember that I needed to reset current state in child directive when view is restored.
Feel free to ignore the issue and focus on v1 and hopefully incorporation of this feature :) Without this mobile app performance is horrible when navigating back and forth. Let me know If I have to refactor anything to match v1 plans.

@ghost
Copy link

ghost commented Dec 8, 2015

Can't wait to see this cached/persisted thing to be merged.........

@FDIM : I have read the your "whole story", your problem is exactly same as mine. How can I get your solution? ( I don't know how to to Git PR , merge , commit , build things :( .. )

@FDIM
Copy link
Author

FDIM commented Dec 8, 2015

Ar the moment for my projects I am referencing my own fork.

If you are using bower you can simply add 'angular-ui-router': 'https://github.com/FDIM/ui-router/#persistent-view' to use it.

#master doesn't include updated dist files, only clean commits for this issue.

@ghost
Copy link

ghost commented Dec 8, 2015

@FDIM : I have managed to download and grunt build your code. It works great but not perfect.

  1. Scroll position sometime not correct (may be because my UI is kinda complex )
  2. The controller is cache even when parameters have changed, is there are way to force reload if params changed ?

Again, thanks for your contribution

@FDIM
Copy link
Author

FDIM commented Dec 8, 2015

2 things you mentioned are the downsides of having a cache as you have to handle these cases manually.

  1. Not sure which element is handling scrolling for you, but in case it is an element within ui-view - scrollTop property is reset when element is removed from DOM to save memory (by browser). You can use my example directive that will bind to scroll event, persist offset in data attribute and set it back once view is restored. Not sure how to handle if it's the body element that is scrolled though.
  2. I feel like that this should be configuration option but in my defense - letting angular update view based on what you change in scope instead of destroying everything and recreating yields better performance. So at current state you have to bind to $viewRestored event and inspect $state.params and do what is needed to update the view with new data.

@ghost
Copy link

ghost commented Dec 8, 2015

@FDIM : got you. thanks again

@stale
Copy link

stale bot commented Jan 24, 2020

This issue has been automatically marked as stale because it has not had
recent activity. It will be closed if no further activity occurs.

This does not mean that the issue is invalid. Valid issues
may be reopened.

Thank you for your contributions.

@stale stale bot added the stale label Jan 24, 2020
@stale stale bot closed this Feb 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preserving view when routed to another view/controller?
2 participants