shepherd: useModalOverlay does not play well with multiple instances on the page

Reduced Test Case Fiddle

Version: whatever version https://shipshapecode.github.io is using right now

// never started 
const unused = new Shepherd.Tour({
  defaultStepOptions: {
    classes: 'shadow-md bg-purple-dark',
    scrollTo: true
  },
  useModalOverlay: true
});

// we start this one
const tour = new Shepherd.Tour({
  defaultStepOptions: {
    classes: 'shadow-md bg-purple-dark',
    scrollTo: true
  },
  useModalOverlay: true
});

tour.addStep('example', {
  title: 'Example Shepherd',
  text: 'Creating a Shepherd is easy too! Just create ...',
  attachTo: '#someImg',
  advanceOn: '.docs-link click'
});

tour.start();

<img src="https://img.kizuna.austinpray.com/images/879bf634e1ea4e63928dc36c4ac3add1.jpg" id="someImg" />

Current Behavior

Active step is not highlighted

note the duplicate shepherdModalOverlayContainers

Expected Behavior

Active step is highlighted

Comments

I would expect shepherd to not touch the dom at all until tour.start()
The insertion of the svg overlay should be idempotent

About this issue

Original URL
State: closed
Created 5 years ago
Reactions: 1
Comments: 22 (12 by maintainers)

Most upvoted comments

I’m curious, why do you have two tours?

It doesn’t really matter if there is a usecase or not: the API you created implies creating multiple instances of a Shepherd.Tour is possible using the “new” keyword.

const tour = new Shepherd.Tour({...}); // create a new _instance_ of the class "Tour"

If this is not what you intended you need to change the API to use a singleton (don’t do this)

Shepherd.Tour.defaultOptions({...}); 
Shepherd.Tour.addStep(...);
Shepherd.Tour.start();

or throw an error if two instances of Shepherd.Tour are instantiated (don’t do this lol)

const tour1 = new Shepherd.Tour({...});
const tour2 = new Shepherd.Tour({...}); // throws new Error('multiple instances of shepherd detected')

By following the current API with a correct understanding of the javascript language you get undefined behavior.

Here is a use case anyway:

Imagine a freshly signed up SaaS user is logging into a single page app backoffice for the first time

// welcomeTour.js
export const welcomeTour = new Shepherd.Tour({/* ... */});
welcomeTour.addStep(/* video intro welcome  */);
welcomeTour.addStep(/* highlight some dashboard items */);
welcomeTour.addStep(/* show them some next steps */);

// exampleFeatureTour.js
export const exampleFeatureTour = new Shepherd.Tour({/* ... */});
exampleFeatureTour.addStep(/* here's what this feature does */);
exampleFeatureTour.addStep(/* click this button to create a new item */);

// app.js
import {welcomeTour} from './welcomeTour.js'
import {exampleFeatureTour} from './exampleFeatureTour.js' // uhoh, at this point we have two instances of Shepherd.Tour and now we have undefined behavior

// use your imagination to insert react-router or some kind of router here, this is pseudocode
if (url === '/dashboard' && needsWelcome) {
  welcomeTour.start();
}
if (url === '/example-feature' && needsExplaining) {
  exampleFeatureTour.start();
}

Having to maintain my own singleton that sets up and tears down a single instance of Shepherd every time I want to show a tour like @michalsnik is doing is definitely not ideal, as he has expressed as well.

So all that leads to:

I would expect shepherd to not touch the dom at all until tour.start()

The insertion of the svg overlay should be idempotent

@rwwagner90 what do you think about these two comments?

austinpray on May 17, 2019