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

specifying {inherit: true} when generating href #13

Closed
wants to merge 1 commit into from

Conversation

fwitzke
Copy link

@fwitzke fwitzke commented May 21, 2014

Hey, the breadcrumb plugin works fine with ui-router 0.2.10. I'm trying to use the latest version because of some other fixes but it doesn't work anymore, and I suspect it will break with next release due to some changes in $state.href method. Anyways, my change does not break the current version and is working with the latest, so I think it doesn't hurt. Let me know if you think this makes sense or not.

@ncuillery
Copy link
Owner

Hi, many thanks for the PR. You are right, angular-breadcrumb is broken when working with master UI-router.

If I understand well, it's due to this PR. In my mind, it introduces a breaking change in UI-router that the UI team has not detected: Don't you think they should change the default value of options.inherit to true ? By doing this, the default behaviour of href method would be as same has before (when the options.inherit was unused).

@fwitzke
Copy link
Author

fwitzke commented May 22, 2014

I think they fixed an issue in href function because it was ignoring the inherit option. Also, I was checking the older versions of the function and it looks like the default option was supposed to be false.

Anyways, I don't have much context on the ui-router code, need to check to see if it makes sense to have it inherited by default, but it looks like yes.

@ncuillery
Copy link
Owner

$state.go has the same option set to true whereas $state.transitionTo has it with false. Maybe I'm wrong but I think $state.href and $state.go are both "high level oriented", and must have the same handiness by inheriting parameters...

I prefer forwarding the problem to the UI-router project. Moreover, we have a real breaking change and I'm probably not the only person concerned.

As you found the problem, I give you the priority to create a PR to UI-Router (If you want of course).

@fwitzke
Copy link
Author

fwitzke commented May 27, 2014

Yeah, I think it makes sense. I can take a look on it but I might take some time, if you want to create a PR before me don't hesitate. Thanks man!

@ncuillery
Copy link
Owner

I'll make it. 0.3.0 seems to be release very soon: https://github.com/angular-ui/ui-router/issues/milestones

@ncuillery
Copy link
Owner

Hi @fwitzke,

For your information, the PR is still open and questions arise as to the health of the project: angular-ui/ui-router#1256

@ncuillery
Copy link
Owner

The ui-router has been merged. The problem won't happen in next ui-router's release.

@ncuillery ncuillery closed this Aug 27, 2014
@fwitzke
Copy link
Author

fwitzke commented Aug 27, 2014

Cool, thanks so much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants