Skip to content

fix(stateDirectives): Generate the proper URL when the state changes #374

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 1 commit into from

Conversation

frapontillo
Copy link

As of now, the generated URL changes if and only if its parameters do. So something
like ui-sref="{{myState}}({id: myId})" would simply work for the first time. When
$scope.myState changes and $scope.myId stays the same, the URL is not regenerated.
This commit fixes this by observing the attribute and regenerating the URL if it changes.
I have also added a test to make sure everything works.
This commit may also be also be relevant for #371, as it allows for empty state values ("")
to be processed and set into the href attribute. We could bind this parameter to a
directive attribute so the user can decide wether to accept this behavior or not.

As of now, the generated URL changes if and only if its parameters do. So something
like `ui-sref="{{myState}}({id: myId})" would simply work for the first time. When
`$scope.myState` changes and `$scope.myId` stays the same, the URL is not regenerated.
This commit fixes this by observing the attribute and regenerating the URL if it changes.
I have also added a test to make sure everything works.
This commit may also be also be relevant for angular-ui#371, as it allows for empty state values (` `)
to be processed and set into the `href` attribute. We could bind this parameter to a
directive attribute so the user can decide wether to accept this behavior or not.
@nateabele
Copy link
Contributor

Other people can chime in here, but I don't think this is desired behavior (see my notes on #139). If you really want something fully dynamic, you're free to combine ng-href with $state.href().

Also, your patch alters an existing test, which, unless you're changing (breaking) existing behavior, should never, ever be done.

@ksperling
Copy link
Contributor

@nateabele Not sure which comment on #139 you're referring to exactly...

I'm kind of surprised we're supporting {{}} in the state name at all. I think it should either (1) support embedded expressions, in which case they should be re-evaluated, or (2) not support them at all, in which case {{myState}} should be taken literally as a (weird) state name.

I think dynamically looking up the state name independently of the parameters is weird enough that we don't need to support it, so I'd tend towards (2) at this point.

@nateabele
Copy link
Contributor

Not sure which comment on #139 you're referring to exactly...

@ksperling: "I know there was some mention of a more dynamic way of referencing states. IMO that is out-of-scope for a shorthand syntax. If people want something more advanced, they can combine ng-href with $state.href()."

I'm kind of surprised we're supporting {{}} in the state name at all.

I'm pretty sure we aren't (which is what this patch is about). If we are, it's certainly not intentional. I'm completely in agreement with option (2).

@frapontillo
Copy link
Author

Ok, I see your points. Actually, what I'm doing is referencing a single variable (myFullState) that includes both the state name and parameters.

Regardless of this specific PR, I think you should somehow support dynamic state referencing, as it's a common expectation from AngularJS attributes and it would come very useful.

Nevertheless, I can now see it will be very straightforward to implement this behaviour by simply using 'ng-href' and the $state service.

@nateabele
Copy link
Contributor

Yup, as you see, it's already fully supported using existing directives and $state services. :-)

@nateabele nateabele closed this Sep 3, 2013
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants