useModalOverlay does not play well with multiple instances on the page

[austinpray]

Reduced Test Case Fiddle

https://jsfiddle.net/h8gwus79/

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

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

[RobbieTheWagner]

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

[michalsnik]

I noticed same issue. According to documentation it should work as each tour created should be a separate thing, but unfortunately it's not. The reason for having few tours is quite simple actually, you might have two different pages that you want to guide user through based on different actions and having service that can initiate both of these respectively without having to add and remove steps while routing between pages would make things much simpler. Unless there is another / easier way for achieving this @rwwagner90 🤔 Happy to hear your thoughts.

[michalsnik]

The way I worked around it for now is by creating tour creator functions, that initiate new tour and set all steps anywhere I want to display the given tour, without pre-inializing them. It works because anytime tour is closed the modal is being removed and then when I init new tour it's being created properly, but having two tours initiated at the same time doesn't work as described above.

[austinpray]

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:

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

@rwwagner90 what do you think about these two comments?

[RobbieTheWagner]

@austinpray you seem to have a firm grasp on the behavior you want. Would you like to submit a PR to support it? We did not create the initial API, and have changed quite a lot from 1.x to 2.x etc, so definitely possible we missed some things.

I have never used multiple tours myself, as I tend to use one and swap the steps out as needed. I think having two active at once really should not be allowed, as you would not be able to do the modal stuff correctly if two tours are running, so if we allow two or more instances, we would have to setup some error handling that ensures only one can be active at a time.

[RobbieTheWagner]

@chuckcarpenter @jaredgalanis do you guys have thoughts on this?

[chuckcarpenter]

@michalsnik I think you could still manage steps based on user actions with the getById() method. I agree though, as @austinpray noted, it implies that you can have more than one tour. It's really just a matter of how we want to address it for stability sake moving forward @rwwagner90. I think making sure the overlay is unique to each instance is a good idea and adding more to stabilize that use case is the path of least resistance over a complete refactor.

[RobbieTheWagner]

@chuckcarpenter do you have ideas on how to support this? If so, and you and @austinpray could work together on a solution, that would be great!

[austinpray]

@rwwagner90 @chuckcarpenter Hey! Thanks for getting back to me on that.

I can send y'all a couple of different PRs:

Fix undefined behavior with useModalOverlay when multiple instances of Shepherd.Tour exist
We could go a couple different ways here

• Make svg overlay dom element insertion idempotent: if one already exists in the dom, just use that one
• Have each instance have its own guaranteed unique svg overlay dom element

I am gonna try both ways and see which one ends up being simpler. This is a backwards compatible change, it will be a patch level semver change. The current behavior is undefined so anything is better than that haha.

Do not touch dom until tour.start()
Side-effects in constructors are a big design smell. Try to limit touching the dom until tour.start() and attempt to leave the dom exactly the way you started when tour is hidden. This could possibly be a minor level semver change but definitely backwards compatible.

[austinpray]

Just to be clear: I'm not talking about modifying the API at all, just removing undefined behavior so the current API works as expected

[chuckcarpenter]

@austinpray I totally agree and think that's the best way forward. Thanks for finding this and the help!

[RobbieTheWagner]

@austinpray @chuckcarpenter I defer to you guys here, as it sounds like you have some good ideas. I am on vacation, and trying to avoid doing much coding until I get back June 3rd 😝

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[RobbieTheWagner]

@austinpray did you ever come up with a solution here? If not, I have some time to take a look now.

[austinpray]

@rwwagner90 I only got as far as familiarizing myself with the internals and then got distracted with work. No code yet. Feel free to take a stab at it! I can work alongside you in a branch if you put one up

[RobbieTheWagner]

@austinpray I just opened a PR that will reuse the existing modal element, if it already exists, this fixes that aspect #414

However, there is probably more to do to work on the not touching the DOM until start bit. Let me know your thoughts on that!

[austinpray]

Will review soon!

[RobbieTheWagner]

@austinpray I'm going to go ahead and merge, and release a new version, but please let me know your thoughts when you get a chance on whether it fixes your issues or we need to do more still.

[austinpray]

Closed by #414

Thank you!

[RobbieTheWagner]

@austinpray you're welcome! Did you want to also pursue having Shepherd not touch the DOM until start or do you think this is sufficient?

[austinpray]

Well this solves the specific issue I was describing. I can make another issue/proposal for not touching dom as an enhancement.

[RobbieTheWagner]

@austinpray sounds good 👍