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

All navigation components should watch for changes in "page" and react accordingly #26

Closed
JoaoSaleiro opened this issue Feb 8, 2014 · 14 comments

Comments

@JoaoSaleiro
Copy link

This is a feature request to improve highly the flexibility of onsenUI in terms of navigation.

All components that perform navigation (ons_screen, ons_navigator, ons_slidingmenu, etc) should watch for changes in "page" (or behind-page, etc) and perform navigation when changes to page happen.

Components that support stack navigation (ons_navigator and ons_screen) should give an extra (bindable) property to set the navigation approach (push, resetToPage, pop).

This small change would give greater flexibility for the coder to build more powerful navigation systems. For example, this would allow the coder to create a NavigationService, with centralized navigation logic, representing the state of all navigation components. Navigation components spread across the app would simply bind to properties in the NavigationService.

@kruyvanna
Copy link
Contributor

All components that perform navigation (ons_screen, ons_navigator, ons_slidingmenu, etc) should watch for changes in "page" (or behind-page, etc) and perform navigation when changes to page happen.

This is great!

Components that support stack navigation (ons_navigator and ons_screen) should give an extra (bindable) property to set the navigation approach (push, resetToPage, pop).

I understand the use case but cannot figure out the syntax to achieve this.
Could you give code examples of how they can be done?

@JoaoSaleiro
Copy link
Author

Now that you ask, it would probably be hard to implement in the way I suggested because it would lead to a race condition hard do solve.

An alternative could be to use just one property (page) that could receive either a string (a pageURL) or an object (with properties explaining how to show the page). Example:

HTML

 <ons-screen page="{{page}}"></ons-screen>

Javascript to show a new page using default method (resetToPage):

app.controller('AppCtrl', function($scope) {
    $scope.navigate = function() {
         $scope.page = 'myPage.html'
      }  
});

Javascript to show a new page using push:

app.controller('AppCtrl', function($scope) {
    $scope.navigate = function() {
            $scope.page = {url: 'myPage.html', method: 'push'};
      }  
});

Javascript to pop/dismiss page:

app.controller('AppCtrl', function($scope) {
    $scope.navigate = function() {
           $scope.page = {method: 'pop'};
      }  
});

And so one. The object could be extended to receive, for example a "transition" property.

If all navigators can simply bind to properties, then a centralized NavigationService would take care of the full application state without knowing about and needing instances of ons.navigators .

BTW, I saw that on the onseenui-screen branch you added watchers for the page in the onsScreen, which I'm already using in the current project. Such a simple change, and it simplified so much the navigation architecture.

@kruyvanna
Copy link
Contributor

Great!! Make sense!

BTW, I saw that on the onseenui-screen branch you added watchers for the page in the onsScreen, which I'm already using in the current project. Such a simple change, and it simplified so much the navigation architecture.

Currently it just reset the page stack. I prefer your example's way.

@kruyvanna
Copy link
Contributor

initial implenentation 98019ea.
Please see branch feature/navigation_binding

also we need to change the page binding to bi-directional binding or page changes will not be notified if used with ons.screen.presentPage/dismissPage

        scope: {
            title: '@',
            page: '='

This also apply to navigator.

To illustrate this. I leave the screen

        scope: {
            title: '@',
            page: '@',

and change navigator to

        scope: {
            title: '@',
            page: '=',

You can test in
*demo/screen_page_binding_demo (presentPage will not work after using ons.screen.dismissPage)

  • demo/navigator_page_binding_demo/ (pushPage works both using page attribute binding and ons.navigator.popPage)

@JoaoSaleiro
Copy link
Author

Great work!
I'm already trying the navigation_binding branch, and here's a small issue: now page expects an object. So the below code won't work:

<ons-navigator
        left-button-icon="fa fa-bars"          
        right-button-icon="fa fa-lg fa-plus"
        page="page1.html">
</ons-navigator>

To make it work, it needs a change from page="page1.html" to page=" 'page1.html' ".
While I prefer it to expect an object so it can bind to a model, it breaks compatibility with previous approach.

@JoaoSaleiro
Copy link
Author

I've refactored my existing code to use the navigation_binding branch.

It looks really amazing so far, with better separation of concerns.

So far, apart from the previous issue, I've found just one issue:

Changes to page are not applied immediately. By some reason, I need to call $scope.$apply() for it to work. I'm not sure if this is an issue, or something in my code. But my code is a pretty simple controller, with one method described below and one object

  // ons-navigator is binded to $scope.page, and $scope.pages is a list of page definitions
  $scope.openAdd = function () {
    $scope.page = $scope.pages.add;
    $scope.$apply();
  }

@JoaoSaleiro
Copy link
Author

Btw, here's how my code looks now (simplified example). I really, really like this approach.
So far, I have the Navigation logic inside a Controller, but I'll later refactor it to one (or several) centralized Service(s), that will take care of making navigation decisions for all navigators.

b.controller('NavigatorCtrl', function ($scope, $timeout) {

  $scope.pages = {
    list: {
      url: 'list.html',
      method: 'push',
      options: {
        title: 'Page1',
        leftButtonIcon: "fa fa-bars",
        rightButtonIcon: "fa fa-lg fa-plus",
        onRightButtonClick: "openAdd()"
      }
    },
    add: {
      url: 'add.html',
      method: 'push',
      options: {
        title: 'Add',
        leftButtonIcon: "fa fa-bars",
        onLeftButtonClick: "goback()"
      }
    }
  }

  $scope.page = $scope.pages.list;

  $scope.openAdd = function () {
    $scope.page = $scope.pages.add;
    $scope.$apply();
  }

  $scope.goback = function () {

    $scope.list.method = "pop";
    $scope.page = $scope.pages.list;
    // One could simply do $scope.page = { method: 'pop'}; , but I want my $scope.page object to be as close to reality as possible

    $scope.$apply();
  }
});

The only thing I'm not 100% comfortable with, is what happens to $scope.page object after a pop (see my comment in the code). A way to workaround it, would be to change ons-navigator to expect an object with a slightly more complex format:

    $scope.page: {
      method: 'pop',
      page: {
          url: 'add.html',
          options: {
              title: 'Add',
              leftButtonIcon: "fa fa-bars",
              onLeftButtonClick: "goback()"
       }
     }

This way I could have a "pages" object with the definition for all pages, that is not dependent to the "method".

The above controller would change to:

 $scope.goback = function () {
    $scope.page = {method: "pop", $scope.pages.list}
}

@JoaoSaleiro
Copy link
Author

Here's another issue: with this syntax, the options.onRightButtonClick should expect a reference to a function and not a String. In other words:

Instead of:

  onRightButtonClick: "openAdd()"

we should have:

  onRightButtonClick: openAdd

@JoaoSaleiro
Copy link
Author

I have made a small change to onRightButtonClicked and onLeftButtonClicked to add support for functions (instead of strings) inside the right/left button click callback.

Here's the final function:

         onLeftButtonClicked: function () {
            console.log('left button clicked');
            var onLeftButtonClick = this.getCurrentNavigatorItem().options.onLeftButtonClick;


            if (onLeftButtonClick) {

              // These two lines are new. If we have a function, we simply call it instead of parsing it
              if (onLeftButtonClick && {}.toString.call(onLeftButtonClick) === '[object Function]') {
                onLeftButtonClick();


              } else {

                var onLeftButtonClickFn = $parse(onLeftButtonClick);
                if (onLeftButtonClick.indexOf('ons.navigator.') >= 0) {
                  onLeftButtonClickFn(scope);
                } else {
                  onLeftButtonClickFn(scope.$parent);
                }
              }
            } else {
              if (this.canPopPage()) {
                scope.ons.navigator.popPage();
              }
            }
          },

This change improves greatly the flexibility of ons-navigator. Now, instead of using:

  add: {
      url: 'page1.html',
      method: 'push',
      options: {
        onLeftButtonClick: "goback()"
      }
    }

we can now use:

  add: {
      url: 'page1.html',
      method: 'push',
      options: {
        onLeftButtonClick: goback
      }
    }

The difference, is that for the first case ons-navigator will look for goback() inside the controller that holds ons-navigator. In the second case, goback() can be defined anywhere. Thanks to this, now it's pretty simply to write a Service that takes care of all navigation, and can be injected anywhere. Other Controllers don't need to know or access the navigator.

I'll paste here a sample of my Service when it's finished.

@JoaoSaleiro
Copy link
Author

In the changes I made above to ons-navigator, it's missing a scope.$apply() after the onLeftButtonClick().

So:

if (onLeftButtonClick && {}.toString.call(onLeftButtonClick) === '[object Function]') {
              onLeftButtonClick();
              scope.$apply()
}

@kruyvanna
Copy link
Contributor

Great work!!
@JoaoSaleiro how about doing Pull Request?
I guess it's easier for you.

Anyway, either way is fine for me.
If you are in, pls PR to dev branch.

@JoaoSaleiro
Copy link
Author

As promised, here's how I'm coding my NavigationService. It looks really great, since I can inject the service anywhere, and easily manage navigation.

NavigationService.js

app.service('NavigationService', function ($translate) {

  var $public = this;

  $public.openAdd = function () {
    previousPage = $public.page;
    $public.page = pages.add;
  }

  $public.goback = function () {
    if (previousPage) {
      previousPage.method = 'pop';
      $public.page = previousPage;
    }
  }

  this.openMenu = function () {
    // TODO: I still have to find a way to fix this, since in this service we don't have a reference to the slidingMenu. Maybe the slidingMenu should also expose a bindable property for the state of the menu
    //ons.slidingMenu.toggleMenu();
  }


  pages = {
    list: {
      url: 'transactions_list.html',
      method: 'push',
      options: {
        title: $translate('TRANSACTIONS'),
        leftButtonIcon: "fa fa-bars",
        rightButtonIcon: "fa fa-lg fa-plus",
        onRightButtonClick: $public.openAdd,
        onLeftButtonClick: $public.openMenu
      }
    },
    add: {
      url: 'transactions_add.html',
      method: 'push',
      options: {
        title: $translate('NEW_TRANSACTION'),
        onLeftButtonClick: $public.goback
      }
    },
    chooseBankAccount: {
      url: 'transactions_add_chooseBankAccount.html',
      method: 'push',
      options: {
        title: $translate('SELECT_ACCOUNT'),
        onLeftButtonClick: $public.goback
      }
    }
  }

  var previousPage = null;
  $public.page = pages.list;
});

Navigator1.html

<ons-navigator
        ng-controller="NavigatorCtrl"
        page="NavigationModel.page">

</ons-navigator>

NavigatorCtrl.js

app.controller('NavigatorCtrl', function ($scope, NavigationService) {
  $scope.NavigationService = NavigationService;
});

If at any moment I need, for example, to navigate to the add page I simply:

1- Inject the NavigationService
2- Call NavigationService.openAdd()

I can easily extend the logic of the NavigationService to evaluate stuff, intercept navigation, etc.

And so on :)

Thanks for all the work put in onseenui. I hope this approach is applied to the rest of the components, so they can all bind to properties and react to Model changes.

@JoaoSaleiro
Copy link
Author

@kruyvanna , about the pull requests: Github is completely new to me and I have a tight deadline for the current project that needs to be closed in a couple of days. Yes, I'm going full on with the onsenui-branch in a project that will enter soon in production :o)

When this project is finished I'll dig a bit on Github and learn how to do pull requests, to simplify contributions. In the meantime hope you don't mind if I keep making suggestions using the comments in issues :)

@JoaoSaleiro
Copy link
Author

For reference purposes, my NavigationService had an issue: for a page stack bigger than two pages, it was not going back. So here's the change I did.
This is not that important for onsenui, but in case someone wants to follow my approach, here's my code:

app.service('NavigationService', function ($translate) {

  var $public = this;

  $public.openAdd = function () {
    push(pages.add);
  }

  $public.openSelectAccount = function () {
    push(pages.selectAccount);
  }

  function push(page) {
    page.method = 'push';
    $public.page = page;
    pageStack.push(page);
  }

  $public.pop = function () {
    // Cannot pop the first page
    if (pageStack.length < 2)
      return;

    pageStack.pop();
    var prevPage = pageStack[pageStack.length - 1];
    prevPage.method = 'pop';
    $public.page = prevPage;
  }

  this.openMenu = function () {
    // TODO: I still have to find a way to fix this, since in this service we don't have a reference to the slidingMenu. Maybe the slidingMenu should also expose
    // a bindable property for toggling ???
    //ons.slidingMenu.toggleMenu();
  }


  var pages = {
    list: {
      url: 'transactions_list.html',
      method: 'push',
      options: {
        title: $translate('TRANSACTIONS'),
        leftButtonIcon: "fa fa-bars",
        rightButtonIcon: "fa fa-lg fa-plus",
        onRightButtonClick: $public.openAdd,
        onLeftButtonClick: $public.openMenu
      }
    },
    add: {
      url: 'transactions_add.html',
      method: 'push',
      options: {
        title: $translate('NEW_TRANSACTION'),
        onLeftButtonClick: $public.pop
      }
    },
    selectAccount: {
      url: 'transactions_account_select.html',
      method: 'push',
      options: {
        title: $translate('SELECT_ACCOUNT'),
        onLeftButtonClick: $public.pop
      }
    }
  }

  $public.page = pages.list;
  var pageStack = [$public.page];

});

@lock lock bot added the outdated label May 18, 2018
@lock lock bot locked as resolved and limited conversation to collaborators May 18, 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