fixed issue when passing false to activeClass would break things

emberjs · Jun 1, 2017 · 53d3ef8 · 53d3ef8
1 parent e4f4b8f
commit 53d3ef8
Show file tree

Hide file tree

Showing 2 changed files with 57 additions and 10 deletions.
diff --git a/packages/ember-glimmer/lib/components/link-to.js b/packages/ember-glimmer/lib/components/link-to.js
@@ -543,7 +543,7 @@ const LinkComponent = EmberComponent.extend({
     }
   }),
 
-  _computeActive(routerState) {
+  _isActive(routerState) {
     if (get(this, 'loading')) { return false; }
 
     let routing = get(this, '_routing');
@@ -557,7 +557,7 @@ const LinkComponent = EmberComponent.extend({
 
     for (let i = 0; i < currentWhen.length; i++) {
       if (routing.isActiveForRoute(models, resolvedQueryParams, currentWhen[i], routerState, isCurrentWhenSpecified)) {
-        return get(this, 'activeClass');
+        return true;
       }
     }
 
@@ -578,31 +578,37 @@ const LinkComponent = EmberComponent.extend({
     @property active
     @private
   */
-  active: computed('attrs.params', '_routing.currentState', function computeLinkToComponentActive() {
+  active: computed('attrs.params', '_active', function computeLinkToComponentActiveClass() {
     let currentState = get(this, '_routing.currentState');
     if (!currentState) { return false; }
 
-    return this._computeActive(currentState);
+    return this.get('_active') ? get(this, 'activeClass') : false;
+  }),
+
+  _active: computed('_routing.currentState', function computeLinkToComponentActive() {
+    let currentState = get(this, '_routing.currentState');
+    if (!currentState) { return false; }
+    return this._isActive(currentState);
   }),
 
   willBeActive: computed('_routing.targetState', function computeLinkToComponentWillBeActive() {
     let routing = get(this, '_routing');
     let targetState = get(routing, 'targetState');
     if (get(routing, 'currentState') === targetState) { return; }
 
-    return !!this._computeActive(targetState);
+    return this._isActive(targetState);
   }),
 
   transitioningIn: computed('active', 'willBeActive', function computeLinkToComponentTransitioningIn() {
-    if (get(this, 'willBeActive') === true && !get(this, 'active')) {
+    if (get(this, 'willBeActive') === true && !get(this, '_active')) {
       return 'ember-transitioning-in';
     } else {
       return false;
     }
   }),
 
   transitioningOut: computed('active', 'willBeActive', function computeLinkToComponentTransitioningOut() {
-    if (get(this, 'willBeActive') === false && get(this, 'active')) {
+    if (get(this, 'willBeActive') === false && get(this, '_active')) {
       return 'ember-transitioning-out';
     } else {
       return false;

diff --git a/packages/ember/tests/helpers/link_to_test/link_to_transitioning_classes_test.js b/packages/ember/tests/helpers/link_to_test/link_to_transitioning_classes_test.js
@@ -8,7 +8,7 @@ import { setTemplates, setTemplate } from 'ember-glimmer';
 
 let Router, App, registry, container;
 
-let aboutDefer, otherDefer;
+let aboutDefer, otherDefer, newsDefer;
 
 function bootApplication() {
   container.lookup('router:main');
@@ -20,7 +20,11 @@ function assertHasClass(className) {
   while (i < arguments.length) {
     let $a = arguments[i];
     let shouldHaveClass = arguments[i + 1];
-    equal($a.hasClass(className), shouldHaveClass, $a.attr('id') + ' should ' + (shouldHaveClass ? '' : 'not ') + 'have class ' + className);
+    equal(
+      $a.hasClass(className),
+      shouldHaveClass,
+      $a.attr('id') + ' should ' + (shouldHaveClass ? '' : 'not ') + 'have class ' + className
+    );
     i += 2;
   }
 }
@@ -72,6 +76,7 @@ QUnit.module('The {{link-to}} helper: .transitioning-in .transitioning-out CSS c
       Router.map(function() {
         this.route('about');
         this.route('other');
+        this.route('news');
       });
 
       App.AboutRoute = Route.extend({
@@ -88,13 +93,28 @@ QUnit.module('The {{link-to}} helper: .transitioning-in .transitioning-out CSS c
         }
       });
 
-      setTemplate('application', compile('{{outlet}}{{link-to \'Index\' \'index\' id=\'index-link\'}}{{link-to \'About\' \'about\' id=\'about-link\'}}{{link-to \'Other\' \'other\' id=\'other-link\'}}'));
+      App.NewsRoute = Route.extend({
+        model() {
+          newsDefer = RSVP.defer();
+          return newsDefer.promise;
+        }
+      });
+
+      setTemplate('application', compile(`
+        {{outlet}}
+        {{link-to 'Index' 'index' id='index-link'}}
+        {{link-to 'About' 'about' id='about-link'}}
+        {{link-to 'Other' 'other' id='other-link'}}
+        {{link-to 'News' 'news' activeClass=false id='news-link'}}
+      `));
     });
   },
 
   teardown() {
     sharedTeardown();
     aboutDefer = null;
+    otherDefer = null;
+    newsDefer = null;
   }
 });
 
@@ -119,6 +139,27 @@ QUnit.test('while a transition is underway', function() {
   assertHasClass('ember-transitioning-out', $index, false, $about, false, $other, false);
 });
 
+QUnit.test('while a transition is underway with activeClass is false', function() {
+  expect(18);
+  bootApplication();
+
+  let $index = jQuery('#index-link');
+  let $news = jQuery('#news-link');
+  let $other = jQuery('#other-link');
+
+  run($news, 'click');
+
+  assertHasClass('active', $index, true, $news, false, $other, false);
+  assertHasClass('ember-transitioning-in', $index, false, $news, true, $other, false);
+  assertHasClass('ember-transitioning-out', $index, true, $news, false, $other, false);
+
+  run(newsDefer, 'resolve');
+
+  assertHasClass('active', $index, false, $news, false, $other, false);
+  assertHasClass('ember-transitioning-in', $index, false, $news, false, $other, false);
+  assertHasClass('ember-transitioning-out', $index, false, $news, false, $other, false);
+});
+
 QUnit.test('while a transition is underway with nested link-to\'s', function() {
   expect(54);