Skip to content

feat(stateDirectives): specify options in ui-sref for $state calls #644

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

Closed
wants to merge 2 commits into from

Conversation

mfield
Copy link

@mfield mfield commented Dec 6, 2013

A slightly different take on #628. Although @tobigit's regex is quite impressive, I thought it might be nice to let angular do more of the heavy lifting.

Not 100% sure about the change to $StateActiveDirective. Did I understand the second argument to $state.get correctly? Also, should I be passing params straight into $$setStateInfo, or should I find a way to resolve parameter inheritance first? I don't immediately see a way to do that without duplicating the logic in $state.transitionTo. With a little more guidance here I'd be glad to add a couple tests to cover this.

I would be more inclined to spyOn($state, 'href') and spyOn($state, 'go') instead of munging arguments and inspecting the side effects. I went with the latter style since it seemed to match the rest of the spec better.

@timkindberg
Copy link
Contributor

After a quick review this is definitely more full featured, allowing the options to be watched as a scope object, that's pretty cool. Not exactly sure how necessary it is, but it could be nice. Do you have any real use cases for it? Or was it just because it only took a couple extra lines of code to add that functionality in? Your test case doesn't seem like a great use case (switching url to be absolute on the fly).

Also I'm a bit worried about the confusion that users will have about whether they are supplying options for $state.go or $state.href, because they have some same but also some different options. The fact that you use "absolute" in your "parameter change" test makes me think you are utilizing the options to generate the href with $state.href, but I think a lot of people will think they are supplying transitionTo options...

Sorry for rambling just trying to give some thoughts...

@mfield
Copy link
Author

mfield commented Dec 6, 2013

@timkindberg thanks for the thoughts, they made me think some more on it. My actual use case is #395 -- this is more of a nudge in that direction (remove the double quotes from line 7 and that's one possible implementation). The test cases themselves are pretty arbitrary and were chosen for their easily observable side-effects.

As it stands, the parameters are for both href and go. "inherit" and "relative" have the same meaning to both functions. Always happy to clean this up if you have any suggestions or requirements.

One thing I noticed is that it's not very defensive. I put in a couple of specs for "silly" things you could do. Not sure if they're any cause for concern.

@nateabele
Copy link
Contributor

Yeah, I don't think this is the route we want to go. What I was contemplating was having a ui-sref-options attribute to allow an applicable/safe subset of the options of go()/transitionTo().

I agree there should be a way to reference states dynamically, and I'm still trying to think of a good syntax to allow that, but this isn't it. Fortunately, if you need it that badly in the meantime, it's pretty trivial to roll your own directive.

@timkindberg
Copy link
Contributor

Yeeaaaaah.. man. You rock regardless @mfield!

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.

3 participants