Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add skip event listener #57

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PedroLourenco
Copy link

No description provided.

@mohebifar
Copy link
Owner

Thanks for the PR. It looks good, however, I think it'd be better if we keep it firing stop, so it would be compatible with the older versions. To detect whether it was skipped or not, we could pass a flag like wasSkipped? to the event listener along with the step that the tutorial was left at.

@PedroLourenco
Copy link
Author

PedroLourenco commented Aug 3, 2018

@mohebifar thanks for the feedback.
The wasSkipped flag is a good ideia but i don't understand why I need to pass the skipped step. I can get the skipped step by checking the current step. Am I missing something.

Could you please explain me your idea?
Thanks

@mohebifar
Copy link
Owner

@PedroLourenco

eventEmitter.addEventListener('stop', ({ wasSkipped, step }) => {
    if (wasSkipped) {
        // was skipped at step: `step`
    }
})

Let me know what you think

@PedroLourenco
Copy link
Author

@mohebifar
I need the skippedStep so i can navigate to that screen and I also need the nextStep so i can start the tour from that step.

eventEmitter.addEventListener('stop', ({ wasSkipped, skippedStep, nextStep }) => {
    if (skippedStep) {
        // was skipped at step: `skippedStep`
    }
})

and because i only have the skippedStep and nextStep if the wasSkipped === true i can change it to

eventEmitter.addEventListener('stop', ({ skippedStep, nextStep }) => {
    if (skippedStep) {
        // was skipped at step: `skippedStep`
    }
})

What you think about it?

@mohebifar
Copy link
Owner

That sounds good! But I'd say let's keep wasSkipped.

Would like to do that? If so, feel free to update this PR, otherwise, we can close it and I'll try to implement it later.

@PedroLourenco
Copy link
Author

Hi @mohebifar I'm happy to do it.
Thanks for the feedback.

@RichardLitt
Copy link
Contributor

@mohebifar Can you review this again, please?

@mohebifar
Copy link
Owner

Where is wasSkipped passed into handleStop?

@PedroLourenco
Copy link
Author

File src/components/CopilotModal.js line 215.

handleStop = (wasSkipped?: boolean) => {

@mohebifar
Copy link
Owner

mohebifar commented Nov 23, 2018

I tested your branch and it doesn't seem to pass wasSkipped correctly until you change the skip button's code in Tooltip.js to the following:

      {
        !isLastStep ?
          <TouchableOpacity onPress={() => handleStop(true)}>
            <Button>Skip</Button>
          </TouchableOpacity>
          : null
      }

Couldn't we just use this.props.isLastStep in CopiotModal to detect the skip action instead of passing an arg to the stop button? Something like this:

handleStop = (wasSkipped?: boolean) => {
    const wasSkipped = !this.props.isLastStep;
    this.reset();
    this.props.stop(wasSkipped);
}

@zer0nim
Copy link

zer0nim commented Dec 13, 2018

Hi,
I'm also looking for this feature, it will be great to be able to differentiate skip and stop button.

But the problem with the @mohebifar solution:

const wasSkipped = !this.props.isLastStep;

is we can't put a skip button on the last step.

I know this is weird to put a skip button on the last step but for example, it can be useful if you want to use the "skip" button for another behavior than simply stoping the walkthrough.

Thank's

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants