Improve CodeClimate and test coverage.

shipshapecode · Oct 24, 2018 · 3b6552f · 3b6552f
1 parent 05fd17a
commit 3b6552f
Show file tree

Hide file tree

Showing 3 changed files with 68 additions and 47 deletions.
diff --git a/src/js/tour.js b/src/js/tour.js
@@ -153,6 +153,10 @@ export class Tour extends Evented {
     }
   }
 
+  isActive() {
+    return Shepherd.activeTour === this;
+  }
+
   /**
    * Go to the next step in the tour
    * If we are at the end, call `complete`
@@ -218,30 +222,25 @@ export class Tour extends Evented {
    * @param {Boolean} forward True if we are going forward, false if backward
    */
   show(key = 0, forward = true) {
-    if (this.currentStep) {
-      this.currentStep.hide();
-    }
-
-    this._setupActiveTour();
-
     const step = isString(key) ? this.getById(key) : this.steps[key];
 
-    if (!step) {
-      return;
-    }
+    if (step) {
+      this._updateStateBeforeShow();
 
-    const shouldSkipStep = isFunction(step.options.showOn) && !step.options.showOn();
-    // If `showOn` returns false, we want to skip the step, otherwise, show the step like normal
-    if (shouldSkipStep) {
-      this._skipStep(step, forward);
-    } else {
-      this.trigger('show', {
-        step,
-        previous: this.currentStep
-      });
+      const shouldSkipStep = isFunction(step.options.showOn) && !step.options.showOn();
 
-      this.currentStep = step;
-      step.show();
+      // If `showOn` returns false, we want to skip the step, otherwise, show the step like normal
+      if (shouldSkipStep) {
+        this._skipStep(step, forward);
+      } else {
+        this.trigger('show', {
+          step,
+          previous: this.currentStep
+        });
+
+        this.currentStep = step;
+        step.show();
+      }
     }
   }
 
@@ -252,6 +251,7 @@ export class Tour extends Evented {
     this.trigger('start');
 
     this.currentStep = null;
+    this._setupActiveTour();
     this.next();
   }
 
@@ -281,6 +281,16 @@ export class Tour extends Evented {
   _setTooltipDefaults() {
     tippy.setDefaults(tooltipDefaults);
   }
+
+  _updateStateBeforeShow() {
+    if (this.currentStep) {
+      this.currentStep.hide();
+    }
+
+    if (!this.isActive) {
+      this._setupActiveTour();
+    }
+  }
 }
 
 export { Shepherd };
diff --git a/src/js/utils.js b/src/js/utils.js
@@ -2,6 +2,26 @@ import { isString, isObjectLike, isUndefined, zipObject } from 'lodash';
 import tippy from 'tippy.js';
 import { missingTippy } from './utils/error-messages';
 
+const centeredStylePopperModifier = {
+  computeStyle: {
+    enabled: true,
+    fn(data) {
+      data.styles = Object.assign({}, data.styles, {
+        left: '50%',
+        top: '50%',
+        transform: 'translate(-50%, -50%)'
+      });
+
+      return data;
+    }
+  }
+};
+
+// Used to compose settings for tippyOptions.popperOptions (https://atomiks.github.io/tippyjs/#popper-options-option)
+const defaultPopperOptions = {
+  positionFixed: true
+};
+
 /**
  * TODO rewrite the way items are being added to use more performant documentFragment code
  * @param html
@@ -135,16 +155,11 @@ function _makeAttachedTippyOptions(attachToOptions) {
     ...this.options.tippyOptions
   };
 
-  // Build the proper settings for tippyOptions.popperOptions (https://atomiks.github.io/tippyjs/#popper-options-option)
-  const popperOptsToMerge = {
-    positionFixed: true
-  };
-
   if (this.options.tippyOptions && this.options.tippyOptions.popperOptions) {
-    Object.assign(popperOptsToMerge, this.options.tippyOptions.popperOptions);
+    Object.assign(defaultPopperOptions, this.options.tippyOptions.popperOptions);
   }
 
-  resultingTippyOptions.popperOptions = popperOptsToMerge;
+  resultingTippyOptions.popperOptions = defaultPopperOptions;
 
   return resultingTippyOptions;
 }
@@ -164,32 +179,18 @@ function _makeCenteredTippy() {
     ...this.options.tippyOptions
   };
 
-  const popperOptsToMerge = {
-    positionFixed: true
-  };
-
   tippyOptions.arrow = false;
   tippyOptions.popperOptions = tippyOptions.popperOptions || {};
 
   const finalPopperOptions = Object.assign(
     {},
-    popperOptsToMerge,
+    defaultPopperOptions,
     tippyOptions.popperOptions,
     {
-      modifiers: Object.assign({
-        computeStyle: {
-          enabled: true,
-          fn(data) {
-            data.styles = Object.assign({}, data.styles, {
-              left: '50%',
-              top: '50%',
-              transform: 'translate(-50%, -50%)'
-            });
-
-            return data;
-          }
-        }
-      }, tippyOptions.popperOptions.modifiers)
+      modifiers: Object.assign(
+        { centeredStylePopperModifier },
+        tippyOptions.popperOptions.modifiers
+      )
     }
   );
 

diff --git a/test/unit/test.step.js b/test/unit/test.step.js
@@ -356,7 +356,7 @@ describe('Tour | Step', () => {
   });
 
   describe('setupElements()', () => {
-    it('calls destroy if element is already set', () => {
+    it('calls destroy on the step if the content element is already set', () => {
       const step = new Step();
       let destroyCalled = false;
       step.el = document.createElement('a');
@@ -365,6 +365,16 @@ describe('Tour | Step', () => {
       assert.isOk(destroyCalled, 'setupElements method called destroy with element set');
     });
 
+    it('calls destroy on the tooltip if already exists', () => {
+      const step = new Step();
+      let destroyCalled = false;
+      step.tooltip = {
+        destroy() { destroyCalled = true; }
+      };
+      step.setupElements();
+      assert.equal(destroyCalled, true, 'setupElements method called destroy on the existing tooltip');
+    });
+
     it('calls bindAdvance() if advanceOn passed', () => {
       const step = new Step({
         next: () =>  true