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

ui-sref-active does not work with state that have an array as a parameter #2696

Closed
tommysanterre opened this issue Apr 15, 2016 · 12 comments
Closed
Labels
Milestone

Comments

@tommysanterre
Copy link

Given a state with param of type array

$stateProvider.state('stateA', {
    params: {
      paramA: {
        value: [],
        array: true,
        type: 'int',
       },
    },

The active class is not added on the html element when we navigate to the state

<a ui-sref-active="active" ui-sref="stateA({paramA: [1,2]})">Link</a>
@tommysanterre
Copy link
Author

tommysanterre commented Apr 21, 2016

I have also tried creating a custom type. But this does not work as well

$urlMatcherFactoryProvider.type('intArray', {
  encode: (val) => {
    return val.join(',');
  },
  decode: (string) => {
    return string.split(',').map(s => parseInt(s, 10));
  },
  is: (val) => {
    return Array.isArray(val) && val.every(Number.isInteger);
  },
  pattern: /\d*(?:,\d+)*/,
  equals: (a, b) => {
    return angular.equals(a, b);
  },
});

@ZuBB
Copy link

ZuBB commented Jun 7, 2016

I can confirm this issue. but I have one correction to it. ui-sref-active does not work if ui-sref has any params. see next diff

vasyl@4540s ~/work/current/atv/src/js/dashboard $ gt di
diff --git a/src/js/campaign/direct-buy/report/module/report.html b/src/js/campaign/direct-buy/report/module/report.html
index 2a823ff..a14ec69 100644
--- a/src/js/campaign/direct-buy/report/module/report.html
+++ b/src/js/campaign/direct-buy/report/module/report.html
@@ -4,11 +4,11 @@
       <div cad-column="49">
         <cad-tabs type="orange" class="hidden-print">
           <cad-tabs-item ui-sref-active="is-active"
-                         ui-sref="advancedtv.campaign.directbuy.report.pacing({campaignData: vm.data})"
+                         ui-sref="advancedtv.campaign.directbuy.report.pacing"
                          text="atv.directBuy.report.tabs.pacing_report">
           </cad-tabs-item>
           <cad-tabs-item ui-sref-active="is-active"
-                         ui-sref="advancedtv.campaign.directbuy.report.rollup({campaignData: vm.data})"
+                         ui-sref="advancedtv.campaign.directbuy.report.rollup"
                          text="atv.directBuy.report.tabs.rollup_file">
           </cad-tabs-item>
         </cad-tabs>
vasyl@4540s ~/work/current/atv/src/js/dashboard $ 

with passed params ui-sref-active does not work. without all is OK

@christopherthielen
Copy link
Contributor

This is a bug in $state.includes.
This code should be updated to use typed Param comparison, not object equality

      return params ? equalForKeys(state.params.$$values(params), $stateParams, objectKeys(params)) : true;

This code does == comparison, which obviously doesn't work for arrays.

function equalForKeys(a, b, keys) {
  if (!keys) {
    keys = [];
    for (var n in a) keys.push(n); // Used instead of Object.keys() for IE8 compatibility
  }

  for (var i=0; i<keys.length; i++) {
    var k = keys[i];
    if (a[k] != b[k]) return false; // Not '===', values aren't necessarily normalized
  }
  return true;
}

@christopherthielen
Copy link
Contributor

@christopherthielen christopherthielen added this to the 0.3.2 milestone Jun 7, 2016
@ZuBB
Copy link

ZuBB commented Jun 7, 2016

@christopherthielen could you please post here a ping message when 0.3.2 will be released?

@ZuBB
Copy link

ZuBB commented Oct 10, 2016

Hi there!

@christopherthielen do you have any news on this?

Thanks in advance

@zeusdeux
Copy link

zeusdeux commented Oct 11, 2016

@ZuBB Could you PR this please? It would unblock both you and I since we are both waiting on 0.3.2. Cheers!

/cc @christopherthielen

@ZuBB
Copy link

ZuBB commented Oct 11, 2016

@zeusdeux I am sorry I am a bit busy right now and quite possibly for next month too. maybe after that.

christopherthielen added a commit that referenced this issue Nov 3, 2016
…ing `==`)

- Allows .includes() to match when a param value is non-primitive and == is not useful (such as an array)

Closes #2696
@christopherthielen
Copy link
Contributor

closed by 6958c24

@christopherthielen
Copy link
Contributor

0.3.2 released

@joaovieira
Copy link

You should be careful with these changes. For us this was a breaking change as we were using null to unset/reset params. With the previous equality check undefined == null // true, while with the strict equality this fails undefined === null // false.

I've also noticed the change was only applied to the $state.includes and not in $state.is. Could this bring inconsistent behaviours?

@christopherthielen
Copy link
Contributor

@joaovieira sorry about that. It's hard to tell which broken behaviors people rely on "in the wild". But the old behavior was definitely wrong, because it didn't work at all for object typed parameters. It was also inconsistent with the parameter equality checking in transitionTo. I wonder how this affected your app?

strict equality

Hmm, this delegates to the parameter's type. The "string" type (default type) uses loose equality == so that's interesting that it broke for you.

I've also noticed the change was only applied to the $state.includes and not in $state.is. Could this bring inconsistent behaviours?

Definitely, that is a problem. I will open a new issue to track it. #3154 Thanks for reporting it!

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

No branches or pull requests

5 participants