Increase test coverage, refactor, and cleanup (#224)

* Add test for undefined popper

* Various cleanup

* Add _mergePopperOptions function

* Add tests for removing specific handler with `off`

* Bump deps, yarn upgrade

package.json

@@ -44,7 +44,7 @@
     "popper.js": "^1.14.3"
   },
   "devDependencies": {
-    "autoprefixer": "^9.1.0",
+    "autoprefixer": "^9.1.3",
     "babel-core": "^6.26.0",
     "babel-loader": "^7.1.4",
     "babel-plugin-add-module-exports": "^0.2.1",
@@ -63,14 +63,15 @@
     "eslint-plugin-mocha": "^5.1.0",
     "eslint-plugin-ship-shape": "^0.6.0",
     "extract-loader": "^2.0.1",
-    "file-loader": "^1.1.11",
+    "file-loader": "^2.0.0",
     "glob": "^7.1.2",
     "http-server": "^0.11.1",
+    "inject-loader": "^4.0.1",
     "istanbul-instrumenter-loader": "^3.0.1",
     "karma": "^3.0.0",
     "karma-chrome-launcher": "^2.2.0",
     "karma-coverage": "^1.1.2",
-    "karma-coverage-istanbul-reporter": "^2.0.1",
+    "karma-coverage-istanbul-reporter": "^2.0.2",
     "karma-mocha": "^1.3.0",
     "karma-mocha-reporter": "^2.2.5",
     "karma-sourcemap-loader": "^0.3.7",
@@ -87,9 +88,9 @@
     "stylelint-config-ship-shape": "^0.4.0",
     "stylelint-webpack-plugin": "^0.10.5",
     "uglifyjs-webpack-plugin": "^1.3.0",
-    "webpack": "^4.16.3",
+    "webpack": "^4.17.1",
     "webpack-cli": "^3.1.0",
-    "webpack-dev-server": "^3.1.5"
+    "webpack-dev-server": "^3.1.6"
   },
   "engines": {
     "node": ">= 6.*"

src/js/tour.js

@@ -177,15 +177,13 @@ export class Tour extends Evented {
     }
   }
 
+  /**
+   * Show a specific step in the tour
+   * @param {Number|String} key The key to look up the step by
+   * @param {Boolean} forward True if we are going forward, false if backward
+   */
   show(key = 0, forward = true) {
-    if (this.currentStep) {
-      this.currentStep.hide();
-    } else {
-      document.body.classList.add('shepherd-active');
-      this.trigger('active', { tour: this });
-    }
-
-    Shepherd.activeTour = this;
+    this._setupActiveTour();
 
     const next = _.isString(key) ? this.getById(key) : this.steps[key];
 
@@ -217,6 +215,22 @@ export class Tour extends Evented {
     this.currentStep = null;
     this.next();
   }
+
+  /**
+   * If we have a currentStep, the tour is active, so just hide the step and remain active.
+   * Otherwise, make the tour active.
+   * @private
+   */
+  _setupActiveTour() {
+    if (this.currentStep) {
+      this.currentStep.hide();
+    } else {
+      document.body.classList.add('shepherd-active');
+      this.trigger('active', { tour: this });
+    }
+
+    Shepherd.activeTour = this;
+  }
 }
 
 export { Shepherd };

src/js/utils.js

@@ -72,24 +72,34 @@ export function setupPopper() {
     _setupCenteredPopper(opts);
   }
 
-  const popperOpts = Object.assign({}, {
-    placement: attachment,
-    arrowElement: this.el.querySelector('.popper__arrow'),
-    modifiers: opts.modifiers,
-    positionFixed: opts.positionFixed
-  }, this.options.popperOptions);
-
   if (this.popper) {
     this.popper.destroy();
   }
 
   this.el.classList.add('shepherd-element');
+  const popperOpts = _mergePopperOptions.call(this, attachment, opts);
   this.popper = new Popper(opts.element, this.el, popperOpts);
 
   this.target = opts.element;
   this.target.classList.add('shepherd-enabled', 'shepherd-target');
 }
 
+/**
+ * Merge the global popperOptions, and the local opts
+ * @param {String} attachment The direction for attachment
+ * @param {Object} opts The local options
+ * @returns {Object} The merged popperOpts object
+ * @private
+ */
+function _mergePopperOptions(attachment, opts) {
+  return Object.assign({}, {
+    placement: attachment,
+    arrowElement: this.el.querySelector('.popper__arrow'),
+    modifiers: opts.modifiers,
+    positionFixed: opts.positionFixed
+  }, this.options.popperOptions);
+}
+
 /**
  * Sets up a popper centered on the screen, when there is no attachTo element
  * @param {Object} opts The config object

test/.eslintrc.js

@@ -20,6 +20,7 @@ module.exports = {
     Event: true,
     MouseEvent: true,
     expect: false,
+    require: false,
     window: false
   },
   env: {

test/test.evented.js

@@ -1,39 +1,51 @@
-/* global describe,it */
 import { assert } from 'chai';
 import { Evented } from '../src/js/evented';
 
-describe('Evented', function() {
-  const testEvent = new Evented();
-  let testOnTriggered = false;
-  describe('on()', function(){
-    it('adds a new event binding', function(){
-      testEvent.on('testOn', () => testOnTriggered = true );
+describe('Evented', () => {
+  let testEvent, testOnTriggered;
+
+  beforeEach(() => {
+    testEvent = new Evented();
+    testEvent.on('testOn', () => testOnTriggered = true);
+    testOnTriggered = false;
+  });
+
+  describe('on()', () => {
+    it('adds a new event binding', () => {
       assert.ok(testEvent.bindings.testOn, 'custom event added');
     });
   });
 
-  describe('trigger()', function(){
-    it('triggers a created event', function(){
+  describe('trigger()', () => {
+    it('triggers a created event', () => {
       testEvent.trigger('testOn');
       assert.ok(testOnTriggered, 'true is returned from event trigger');
     });
   });
 
-  describe('off()', function(){
-    it('removes an event binding', function(){
+  describe('off()', () => {
+    it('removes a generic event binding when no handler passed', () => {
       testEvent.off('testOn');
       assert.notOk(testEvent.bindings.testOn, 'custom event removed');
     });
 
-    it('does not remove uncreated events', function(){
+    it('removes a specific event binding when handler is passed', () => {
+      const handler = () => {};
+      testEvent.on('testOn', handler);
+      assert.equal(testEvent.bindings.testOn.length, 2, '2 event listeners for testOn');
+      testEvent.off('testOn', handler);
+      assert.equal(testEvent.bindings.testOn.length, 1, '1 event listener for testOn');
+    });
+
+    it('does not remove uncreated events', () => {
       assert.notOk(testEvent.off('testBlank'), 'returns false for non created events');
     });
   });
 
-  describe('once()', function(){
-    it('adds a new event binding that only triggers once', function(){
-      testEvent.once('testOnce', () => true );
-      testEvent.trigger('testOnce')
+  describe('once()', () => {
+    it('adds a new event binding that only triggers once', () => {
+      testEvent.once('testOnce', () => true);
+      testEvent.trigger('testOnce');
       assert.ok(testEvent.bindings.testOnce, 'custom event removed after one trigger');
     });
   });