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

Broken history for longer resolves and menuClose directive #4900

Closed
talamaska opened this issue Jan 4, 2016 · 2 comments
Closed

Broken history for longer resolves and menuClose directive #4900

talamaska opened this issue Jan 4, 2016 · 2 comments

Comments

@talamaska
Copy link

Type: bug

Platform: all

Last working version of the resolve in ui router is ionic 1.0.0 BETA 1

I'm not going to explain how to reproduce because it was already explained in the bugs I referred to.

So this is continuation of the bug report #3941
After a research that I've made, i have found that there is hardcoded timeout in the menuClose directive - a379bfd,
it was introduced to fix
#4145
#4038
#4132

But it actually introduce a bug #3941

On my opinion there are some problems with this solution and the fix for all those bugs can be more elegant. We all know that timeouts are bad practice, especially when the timeout is with hardcoded time and you don't know when the view will be loaded. It seems that we don't have access from outside to the information when the state data was resolved.

First things first

  1. the history is changed when we try to call transition to the same state as we are currently.
    My solution to this is checking the url of the link in the menu and compare it with the current state. If they are the same, we don't add/change $ionicHistory.nextViewOptions.
  2. to preserve the logic of the history, when opening for example nested state or another state that visually opened from the current one and we want to have back button in the next view , we just need to add
$ionicHistory.nextViewOptions({
              historyRoot: false
            });

in the calling "parent" controller. It maybe not a real parent, but logically your navigation starts from somewhere.

I would add a codepen, but I'm not sure how to demonstrate the change in the ionic source code.

That's why I'm going to paste some code here:

IonicModule
.directive('menuClose', ['$ionicHistory', '$timeout', '$state', function($ionicHistory, $timeout, $state) {
  return {
    restrict: 'AC',
    link: function($scope, $element, $attrs ) {
      $element.bind('click', function() {
        var sideMenuCtrl = $element.inheritedData('$ionSideMenusController');
        //console.log($state, $attrs)
        if (sideMenuCtrl) {
          if($attrs.url !== '#'+$state.$current.url.source) {
            $ionicHistory.nextViewOptions({
              historyRoot: true,
              disableAnimate: true,
              expire: 300
            });
          }
          // if no transition in 300ms, reset nextViewOptions
          // the expire should take care of it, but will be cancelled in some
          // cases. This directive is an exception to the rules of history.js
          /*$timeout( function() {
            $ionicHistory.nextViewOptions({
              historyRoot: false,
              disableAnimate: false
            });
          }, 300);*/
          sideMenuCtrl.close();
        }
      });
    }
  };
}]);

I guess the '#' can be added after checking the setting
if html5Mode is true and what's the actual hashPrefix

In the playlist controller (assume sidemenu start app)

.controller('PlaylistsCtrl', function($scope, $ionicHistory) {
  $scope.playlists = [
    { title: 'Reggae', id: 1 },
    { title: 'Chill', id: 2 },
    { title: 'Dubstep', id: 3 },
    { title: 'Indie', id: 4 },
    { title: 'Rap', id: 5 },
    { title: 'Cowbell', id: 6 }
  ];

  $ionicHistory.nextViewOptions({
              historyRoot: false,
              disableAnimate: false
            });
})
@danbucholtz
Copy link
Contributor

Hi @talamaska,

We decided we aren't going to make any changes to this in V1 unless enough of the community wants to see them. We designed navigation without considering the "resolve" functionality of ui-router, so it can lead to challenges. Some times it works, some times it doesn't. V2's architecture will naturally resolve a lot of these problems.

Please let me know if you have any thoughts on the matter. We really appreciate your contributions to Ionic!

Thanks,
Dan

@talamaska
Copy link
Author

talamaska commented May 9, 2016

Hi, @danbucholtz , I got this from your answer on the original bug report.
Well, what can I say, the bug was there from the start basically. I understand that this was not planned to work that way at all. Oh well, guess I live with that, until the official release of Ionic 2. It's shame that i put so much effort for this one year in implementing material design alone and was so close to a great release and now I have to rewrite everything with the v.2.
Thanks for notifying us for your decision.

Btw what do you think about my suggestion of the code altering. Is it going to work that way?
Is it a good idea to fork ionic v.1 and try out those changes. I was thinking about other thing as well - the change of the side menu behavior - moving from left on top of the main view, not moving the main view, like it is in android.

@ionitron-bot ionitron-bot bot locked and limited conversation to collaborators Sep 8, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants