angular-ui / ui-router

The de-facto solution to flexible routing with nested views in AngularJS
http://ui-router.github.io/
MIT License
13.54k stars 3k forks source link

ui-sref-active don't activate for queryParams + reloadOnSearch=false #2049

Closed thiagofelix closed 4 years ago

thiagofelix commented 9 years ago

Scenario: The state defines reloadOnSearch: false You interact with ui-sref directive that only change query param and stay in the same state. The ui-sref-active directive doesn't activate the element because $stateChangeSuccess is never triggered.

Example: http://plnkr.co/edit/cdaAnNXa2WkChlpAGbAq?p=preview

The $StateRefActiveDirective is made upon the $stateChangeSuccess event as the trigger to check if the element should be highlighted or not.

Here are some options I believe could be used to make ui-sref-active support query params change.

Option 1: Include new event $stateChangeSkipped on the state transitioning livecycle that notifies when the current transition was skipped and then listen to it inside the directive to also check if need to activate the current elemen.

Open 2: Trigger $stateChangeSuccess even if the transition was skipped.

Implementation wise both should be simple changes to do, I am curious to hear if people are experience same kind of problems and what are the thoughts about the proposed solution options.

Related Issues:

1752

1675

denislins commented 9 years ago

I think that the inclusion of a new event should be sufficient to resolve this. I just don't really liked the name, I prefer something like $querySearchChanged.

thiagofelix commented 9 years ago

My fear is that $querySearcChanged might not indicate that is something happening related to $state. I was wondering if we can go with $stateParamsChange ?

denislins commented 9 years ago

I liked, it's good to me. But I'm trying an already existent event, as someone told me in #2067. Maybe this can work out for you too.

Klaster1 commented 9 years ago

If this might help you, I've solved the problem by manually broadcasting $stateChangeSuccess to my navigation directive scope. Here's the fragment from the DDO:

controller($scope, $state, $stateParams){
    $scope.$on('$locationChangeSuccess', () => {
        $scope.$broadcast('$stateChangeSuccess', $state.current, $stateParams)
    })
},

Thanks to @denislins for suggesting $locationChangeSuccess.

thiagofelix commented 9 years ago

Hi @Klaster1, thanks for the suggestion. Currently I am working around the issue differently using the angular decorator feature.

$provide.decorator('$state', ['$delegate', '$scope', '$q', function ($delegate, $scope, $q) {
  var transitionTo = $delegate.transitionTo;
  $state.transitionTo = function (to, toParams, options) {
      var from = $state.$current, fromParams = $state.params;

      return transitionTo(to, toParams, options).then(function (result) {
        var skippedStateReload = $state.transition === null;

        if (skippedStateReload) {
            $scope.$broadcast('$stateChangeSuccess', to.self, toParams, from.self, fromParams);
        }
        return $q.when(result);
  })
});
denislins commented 9 years ago

I started listening for $locationChangeSuccess, and it worked just fine for me. But I didn't need to broadcast the $stateChangeSuccess like you guys did... Is there a reason for it?

Klaster1 commented 9 years ago

@denislins , that's strange, because ui-sref-active explicitly listens for it.

denislins commented 9 years ago

That must be the case then, I wanted to implement a lot of custom functionality so I decided not to use this directive and create my own instead.

But this makes me think: Is ui-sref-active supposed to work when only the parameters change? If so, we should change the directive, and make it listen to $locationChangeSuccess instead, no?

thiagofelix commented 9 years ago

If during $locationChangeSuccess the $stateParams are already in sync with query params them it should works fine.

irnc commented 5 years ago

Issue is not reproducible anymore (tested on latest angular-ui-router@1.0.20), it might been fixed in 1.0.

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

This does not mean that the issue is invalid. Valid issues may be reopened.

Thank you for your contributions.