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

fix($breadcrumb): use $stateParams in case of unhierarchical states. #29

Merged
merged 1 commit into from Jul 25, 2014
Merged

fix($breadcrumb): use $stateParams in case of unhierarchical states. #29

merged 1 commit into from Jul 25, 2014

Conversation

henrytao-me
Copy link
Contributor

Hi @ncuillery,
Thanks for your hard working. You have done a great job here.
I added one bug fix for parent URL. Here is my case:

angular.module('app', []).config(function($stateProvider) {
  $stateProvider.state('communities', {
    url: '/',
    controller: 'CommunitiesCtrl',
    templateUrl: 'app/communities/index.html',
    data: {
      ncyBreadcrumbLabel: 'Home'
    }

  }).state('communities-list', {
    url: '/:community',
    controller: 'CommunitiesListCtrl',
    templateUrl: 'app/communities/list/index.html',
    data: {
      ncyBreadcrumbLabel: '{{breadscrumb.name}}',
      ncyBreadcrumbParent: 'communities'
    }

  }).state('communities-detail', {
    url: '/:community/:id',
    controller: 'CommunitesDetailCtrl',
    templateUrl: 'app/communities/detail/index.html',
    data: {
      ncyBreadcrumbLabel: 'post',
      ncyBreadcrumbParent: 'communities-list'
    }
  });
})

Imagine that I have 3 states, look like these:
communities => http://localhost
communities-list => http://localhost/cute-girls or http://localhost/cute-boys...
communities-detail => http://localhost/cute-girls/01...

Because communities-list state need a param :community, so that when I am in app-communities-detail state (for example: http://localhost/cute-girls/01), I can not go back to previous state which is http://localhost/cute-girls.

Checkout my PR. I fixed that by adding $stateParams when you build ncyBreadcrumbLink.

Thanks!

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling d434c1b on henrytao-me:master into 3c19f89 on ncuillery:master.

@ncuillery
Copy link
Owner

Hi, many thanks for reporting (and fixing) !

In your context, the states are functionally hierarchical (Home > Community list> Community detail). So, I suggest you to use hierarchical states by using a real child relation between them, like that:

communities
communities.communities-list
communities.communities-list.communities-detail

or simply:

communities
communities.list
communities.list.detail

By doing that, you will dispose all the power of the UI-router with $stateParams inheritance, nested views, etc. (for the moment you think in the same way as the legacy router). And the ncyBreadcrumbParent property will be no longer needed.

I'm always a bit reticent to keep the module working with this type of situation but... you've done the work so ok ;)

Anyway, I have two notes about the PR:

  • Please revert the changes of the release task, I prefer doing the releases myself ;) The commit must concern only the src file and the dist files (automatically built by the grunt default task).
  • Can you edit the message of the commit in order to fit the AngularJS convention. For this PR, the commit message can be fix($breadcrumb): use $stateParams in case of unhierarchical states.

I know, this project really needs contributing guidelines ;)

I must update the ncyBreadcrumbParent's doc to avoid misleading uses.

@ncuillery
Copy link
Owner

Contributing guide has been added (9457085).

@henrytao-me henrytao-me changed the title Added $stateParams to fix parent URL error fix($breadcrumb): use $stateParams in case of unhierarchical states. Jul 14, 2014
@henrytao-me
Copy link
Contributor Author

Hi @ncuillery,
Thanks for your kindly response. I will fix all of your notes now.
About your suggestion which using hierarchical states, I agree with you that in some context we can use . to take advantage of ui-router. However, in my case, by design, the view is not hierarchical. So I need to put it in the same state level. It turns out that it's a little bit hard to create breadcrumb.
Then, I found this library. This is really big competitive advantage compared with others. Just adding some params, it will be perfect.

Well-done my friend.

@henrytao-me
Copy link
Contributor Author

Job done. Enjoy PR and waiting for your next release. (Y)

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 1c3c05e on henrytao-me:master into b8ead7c on ncuillery:master.

ncuillery added a commit that referenced this pull request Jul 25, 2014
fix($breadcrumb): use $stateParams in case of unhierarchical states.
@ncuillery ncuillery merged commit 4592bd9 into ncuillery:master Jul 25, 2014
@ncuillery
Copy link
Owner

Landed, thanks.

The release arrives soon.

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

Successfully merging this pull request may close these issues.

None yet

3 participants