Fix turning off modal overlay in successive tour starts

Currently, the `showOrHideModal` function is being called for each step on the `'hide' and 'destroy' events. This has the effect of calling it before showing a tour again after closing it. Moving the hide call up to the service itself fixes this.
RobbieTheWagner · Nov 25, 2018 · 5ed276f · 5ed276f
1 parent 774028f
commit 5ed276f
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 23 deletions.
diff --git a/addon/services/tour.js b/addon/services/tour.js
@@ -56,6 +56,7 @@ export default Service.extend(Evented, {
    * @public
    */
   cancel() {
+    this._hideModalOverlay();
     get(this, 'tourObject').cancel();
   },
 
@@ -64,6 +65,7 @@ export default Service.extend(Evented, {
    * @public
    */
   complete() {
+    this._hideModalOverlay();
     get(this, 'tourObject').complete();
   },
 
@@ -72,6 +74,7 @@ export default Service.extend(Evented, {
    * @public
    */
   hide() {
+    this._hideModalOverlay();
     get(this, 'tourObject').hide();
   },
 
@@ -162,7 +165,6 @@ export default Service.extend(Evented, {
     tourObject.on('cancel', bind(this, 'onTourFinish', 'cancel'));
 
     set(this, 'tourObject', tourObject);
-    this._initModalOverlay();
   },
 
   /**
@@ -172,11 +174,11 @@ export default Service.extend(Evented, {
    */
   _setupModalForStep(step) {
     if (!this.modal) {
-      this._showOrHideModal('hide');
+      this._hideModalOverlay();
 
     } else {
       this._styleModalOpeningForStep(step);
-      this._showOrHideModal('show');
+      this._showModalOverlay();
     }
   },
 
@@ -278,7 +280,6 @@ export default Service.extend(Evented, {
       ['hide', 'destroy'].forEach(event => {
         currentStep.on(event, () => {
           unhighlightStepTarget(currentStep);
-          this._showOrHideModal('hide');
         });
       });
 
@@ -320,7 +321,8 @@ export default Service.extend(Evented, {
       this._modalOverlayElem = createModalOverlay();
       this._modalOverlayOpening = getModalMaskOpening(this._modalOverlayElem);
 
-      this._showOrHideModal('hide');
+      // don't show yet -- each step will control that
+      this._hideModalOverlay();
 
       document.body.appendChild(this._modalOverlayElem);
     }
@@ -350,22 +352,26 @@ export default Service.extend(Evented, {
   },
 
   /**
-   * Show or hide the modal
-   * @param {string} showOrHide 'show' or 'hide'
+   * Show the modal overlay
    * @private
    */
-  _showOrHideModal(showOrHide) {
-    const show = showOrHide === 'show';
+  _showModalOverlay() {
+    document.body.classList.add(modalClassNames.isVisible);
 
-    if (show) {
-      document.body.classList.add(modalClassNames.isVisible);
-    } else {
-      document.body.classList.remove(modalClassNames.isVisible);
+    if (this._modalOverlayElem) {
+      this._modalOverlayElem.style.display = 'block';
     }
+  },
 
+  /**
+   * Hide the modal overlay
+   * @private
+   */
+  _hideModalOverlay() {
+    document.body.classList.remove(modalClassNames.isVisible);
 
     if (this._modalOverlayElem) {
-      this._modalOverlayElem.style.display = show ? 'block' : 'none';
+      this._modalOverlayElem.style.display = 'none';
     }
   }
 });
diff --git a/tests/acceptance/ember-shepherd-test.js b/tests/acceptance/ember-shepherd-test.js
@@ -131,14 +131,12 @@ module('Acceptance | Tour functionality tests', function(hooks) {
     test('Displaying the modal during tours when modal mode is enabled', async function(assert) {
       await visit('/');
 
-      const modalOverlay = document.querySelector(`#${modalElementIds.modalOverlay}`);
-
-      assert.ok(modalOverlay, 'modal overlay is present in the DOM');
-      assert.equal(getComputedStyle(modalOverlay).display, 'none', 'modal overlay is present but not displayed before the tour starts');
-      assert.notOk(document.body.classList.contains(modalClassNames.isVisible), `Body has no class of "${modalClassNames.isVisible}" when shepherd is not active`);
+      assert.equal(document.querySelector(`#${modalElementIds.modalOverlay}`), null, 'modal overlay is not present in the DOM before any tour is started');
 
       await click('.toggleHelpModal');
 
+      const modalOverlay = document.querySelector(`#${modalElementIds.modalOverlay}`);
+
       assert.equal(getComputedStyle(modalOverlay).display, 'block', 'modal overlay is present and displayed after the tour starts');
 
       assert.ok(document.body.classList.contains('shepherd-active'), 'Body gets class of shepherd-active, when shepherd becomes active');
@@ -149,13 +147,12 @@ module('Acceptance | Tour functionality tests', function(hooks) {
     test('Hiding the modal during tours when modal mode is not enabled', async function(assert) {
       await visit('/');
 
-      const modalOverlay = document.querySelector(`#${modalElementIds.modalOverlay}`);
-
-      assert.ok(modalOverlay, 'modal overlay is present in the DOM');
-      assert.equal(getComputedStyle(modalOverlay).display, 'none', 'modal overlay is present but not displayed before the tour starts');
+      assert.equal(document.querySelector(`#${modalElementIds.modalOverlay}`), null, 'modal overlay is not present in the DOM before any tour is started');
 
       await click('.toggleHelpNonmodal');
 
+      const modalOverlay = document.querySelector(`#${modalElementIds.modalOverlay}`);
+
       assert.equal(getComputedStyle(modalOverlay).display, 'none', 'modal overlay is present but not displayed after the tour starts');
 
       assert.ok(document.body.classList.contains('shepherd-active'), 'Body gets class of shepherd-active, when shepherd becomes active');