HiDeoo / intro.js-react

Intro.js react wrapper
MIT License
407 stars 58 forks source link

Steps.onBeforeChange should return result of props.onBeforeChange #7

Closed norama closed 7 years ago

norama commented 7 years ago

Otherwise exiting the tutorial:

onBeforeChange = (nextStepIndex) => {
   ...
     this.setState({
          stepsEnabled: false
     });
     return false; // returning false prevents taking next step
};

will result in error from intro.js:

intro.js:874 Uncaught TypeError: Cannot read property 'element' of undefined
    at IntroJs._showElement (intro.js:874)
    at IntroJs._nextStep (intro.js:392)
    at HTMLAnchorElement.nextTooltipButton.onclick (intro.js:1062)

Adding just a return in Steps.onBeforeChange:

  onBeforeChange = element => {
    ...
    if (onBeforeChange) {
      return onBeforeChange(this.introJs._currentStep, element);
    }
  };

solves the problem. Please fix this.

HiDeoo commented 7 years ago

The documentation is stating that returning false in introJs.onbeforechange to prevent a transition is available since intro.js v2.7.0 but it looks like the change didn't make it until intro.js v2.8.0-alpha.1.

I just pushed and merged a PR to use this feature in intro.js-react. The changes allow you to prevent a transition in the onBeforeChange callback by returning false:

onBeforeChange = (stepIndex) => {
  if (stepIndex === 1) {
    return false;
  }
};

I also added a new optional prop named onPreventChange. This callback is called when a transition is prevented so the user can react to this specific case and do whatever he wants, like disabling the steps like you wanted:

onPreventChange = () => {
  this.setState(() => ({ introEnabled: false }));
};

As we're currently at the first alpha of intro.js v2.8.0, I'm not going push a new version of intro.js-react to NPM yet but I'm going wait until at least alpha is over to see if the feature makes it, if some other APIs are not changed and to get a list of all new features planned the next stable version of intro.js as this already contains a breaking change.

If you really want to use the alpha version of intro.js, you can still directly use the master branch of this repository of course.

norama commented 7 years ago

Thanks you very much for your quick reaction, I really appreciate it, started to use the master branch. Your onPreventChange prop solved another error that intro.js returned when I tried to set the state from onBeforeChange before returning false.

I have discovered a minor problem with parametrization: intro.js resets the step index after onBeforeChange returning false, therefore onPreventChange gets called with step index 0.

I solved this problem by storing the step index in Steps.onBeforeChange:

        if (onBeforeChange) {
            const currentStep = this.introJs._currentStep;
            const continueStep = onBeforeChange(currentStep);

            if (continueStep === false && onPreventChange) {
                setTimeout(() => {
                    onPreventChange(currentStep);
                }, 0);
            }

            return continueStep;
        }

However, when later onExit is called then its step index parameter is again 0, but I can solve this locally in my code.

HiDeoo commented 7 years ago

Ho interesting catch, I didn't realize it was getting reset and assumed it was persisted like a "pause" feature.

If that's definitely still the case when it gets released, I'm going to use your workaround to return the "expected" step index. At least, that seems to me this is the only relevant information.

Thanks for all your feedback.