fix: show overlay again after cancel

[mutschler]

If the tour was cancelled at some point the overlay is only shown if you use tour.start() if you want to continue the tour afterwards (tour.next() or tour.show(x) the overlay is missing.

While i get that you should use tour.start() after the tour is done, i don't really want to go through everything from the beginning again if i cancel mid tour... especially not since tour.getCurrentStep() keeps reporting the last step before the tour was cancelled.

This fixes the need of a workaround by manually calling _setupActiveTour and _setupModal when the tour was cancelled and should be resumed.

EDIT: oh well... this is basically a enhanced version of #1237

EDIT2: Since you where talking about "coming up with a good fix" in the pull-request linked above... i'm not sure if this would be considered a "good" fix. It's a fix without changing to much of the existing code.

a proposed "better" fix (which requires a bit more changes to the code) might be:

• move the setup part of tour.start() logic in a _init function
• keep track of the initialized state which then will be reset in _done
• when calling step.show() make sure to check if initialized is still valid, else fire _init again

That would do more or less the same, but IF something should change on the start/setup part it would only require one place to keep updated

[chuckcarpenter]

@mutschler the only issue I'd have with the proposed solution is the assumption that the use case is to always start a tour where you left off. For me, I'd assume start() is always going to start from the beginning. Perhaps we need a better method that would allow you to startFrom? Also, we intend this library to be kind of "dumb" to state/memory in general, so that the implementor can decide how that's tracked in their application. Certainly open to options and a PR, if you're up for it.

[mutschler]

@chuckcarpenter makes sense!
I never ment to change how start() works, since i think it's pretty clear that it start's from the beginning. No issue with that ;)
and im with you as well on the "dump" part regarting state: i don't think the library should keep track of this, an example on how i implemented it currently is provided later.

I've hacked together a quick example of what i ment with the proposal, see: 2862cf9
Basically this won't change anything for start() but would allow you to keep going when calling hide() which i use for "pausing" and then continue with tour.next() or step.show() sinde currentStep is only reset when the tour is done or canceled

But even if the tour was canceled at some time, it would be nice to give the user an option to resume. While i guess you could make that happen through some kind of startFrom on the tour... I thinl you're right: that should be handled by implementation...

Here's how i do the "logic" part of it:

• listen to the show event of a tour and save the step that's shown to a user var ex: lastTourStep
• when the tour is canceled (becuase the user hit esc or quitt) we keep that 'lastTourStep` saved
• if the user hits the help button again, we check if lastTourStep is defined and if so we ask him if he want's to continue where he left of
• if the user choose to continue i call show on the step saved in lastTourStep

This is absolutely fine and i don't think the library should keep track of that BUT this has currently an issue with the modal:
since the modal get's removed when canceled or done and ONLY created in tour.start() so i basiccally do what i did in this PR but inside my own code so when i ask to resume, i run something like this:

 if (!document.querySelector('.shepherd-modal-overlay-container')) {
    lastTourStep.tour._setupModal()
}
lastTourStep.show()

This is the part i think the library should handle

i hope this makes it a little bit clearer what i was talking about and what i propose ;)

EDIT: so tl;dr: not gonna channge how start() works, just make sure the modal and other setup is properly done when calling show() on a step

[haoming29]

Hey there. We found the same issue in developing our product and I'd like to know if this fix can be merged soon. Thanks!

[RobbieTheWagner]

Do we need all the setup or can we just this.tour.modal.show()?

[mutschler]

im not 100% sure since it's been a while since i've wrote this but i the main reason to use _setupActiveTour here instead of just show was to make sure the correct tour is flagged as active and the 'activation' events are triggered.

[RobbieTheWagner]

I'm wondering if we need to set this to null or if we can just show/hide the modal?

[mutschler]

again not 100% sure but i think i did this to make sure we re-attach the modal element to dom since modalContainer.remove(); removes the modal completely from the dom show() will do nothing (since the element is already deleted)

[RobbieTheWagner]

Hmm, should we maybe just hide/show it instead of fully removing? It's odd that the overlay behavior is different than the behavior of the tour itself. If the tour works without calling start again, then we should probably make the modal do the same.

[mutschler]

I get your point. I didn't want to temper to much with how the whole tour works. Also after some thinking I actually came to the conclusion that keeping it the way it is/was is probably the best. If you end/cancel a tour it the modal should be removed since it's not needed anymore. If you restart (or choose to continue) the modal gets initialised again. Keeps the dom clean.

[RobbieTheWagner]

Gotcha. Would you be able to write some tests to ensure this continues working as expected going forward? It also looks like we have some lint issues to fix.

[mutschler]

I've fixed the lint issues.

Regarding the tests: I haven't written any tests for JS so far but i'll give it a try when i'm back on my machine next week. What would you like me to write a test for?

[RobbieTheWagner]

I think we just need a test that covers the workflow you are trying to accomplish like start tour -> advance a couple steps -> end tour -> call show on the step you left off on -> assert that the modal overlay exists and is shown.