Skip to content

Add index to 'show' and 'cancel' events#454

Merged
RobbieTheWagner merged 4 commits intoshipshapecode:masterfrom
genadis:step-index-in-events
Jul 22, 2019
Merged

Add index to 'show' and 'cancel' events#454
RobbieTheWagner merged 4 commits intoshipshapecode:masterfrom
genadis:step-index-in-events

Conversation

@genadis
Copy link
Contributor

@genadis genadis commented Jul 22, 2019

We want to track some basic analytics for the tour, this simple addition to the events helps a lot for tracking, completion, on which steps user cancel, etc.

… event - to allow for more convenient analytics.
src/js/tour.js Outdated
previous: this.currentStep
forward,
previous: this.currentStep,
index: this.steps.indexOf(step)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since step is already passed, wouldn't it make sense to do this.steps.indexOf(step) in your handler function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still will need the forward flag to save some redundant work determining it from current and previous steps.
steps is not publicly declared on the tour interface, in javascript we can do anything of course.. but if it can be publicly used, it should be mentioned (like you have getCurrentStep() method).
tour.steps and options.steps is not array of same objects, especially since we use tour.addStep() to add all the steps at init.

The flow would be something like:

  1. the currentStep is not updated yet on the tour, so we would need to use something like:
tour.on('show', ({step, previous}) => {
  const index = tour.steps.indexOf(step);
  const prevIndex = tour.step.indexOf(previous);
 
  // figure out if there is prevIndex (previous can be undefind), check if index is higher that prev to determine direction, etc... 
});

adding the above will simplify the usage...
There are not many reasons to mess up with tour.steps directly and I guess it's discouraged as well..

Hope the above makes sense...
Thanks

Copy link
Member

@RobbieTheWagner RobbieTheWagner Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tour.steps is fine to use. It is not private. What I am getting at here is this is all possible on your end with the current API. I want to avoid adding extra things where we can.

Is the direction really necessary for logging to your analytics? If you log they viewed step 2, then they viewed step 1, that should be sufficient right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We measure user journey in our analytics, so the direction is important.
If it's an issue, I'll remove it and figure something out, but the index in cancel event is a must, we don't want to start tracking state for such a trivial task..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We measure user journey in our analytics, so the direction is important.

Sure, but what I am saying is if you are logging events for each step in the journey, and you see step 2, step 1 logged, you know they went backwards and there is no need to track forward just the index. Am I missing something there?

the index in cancel event is a must

Yes, that part is fine with me. Since we are destroying all the steps there, it would be difficult to find the index on your own. I am okay with adding this.

Copy link
Contributor Author

@genadis genadis Jul 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated the PR only for the cancel event.

Sure, but what I am saying is if you are logging events for each step in the journey, and you see step 2, step 1 logged, you know they went backwards and there is no need to track forward just the index. Am I missing something there?

we use events based analytics, consider you have events saved in DB that have only the step id. how can you answer a simple question "how many times in the last week (and a bunch of other criteria) user stepped back from step 2?"
If we have this flag on each event it's a simple count query.
We need the index for the logic of calculating the completion rate (going back doesn't count) with having index it's as simple as (index+1)/totalNumOfSteps. And the steps might have meaningful names, not 1,2,3.

No worries, I'll add the logic of finding the index and the direction on the event handling callback.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see. You guys have a lot of complex stuff you are doing, that I have not considered, so thank you for clarifying!

@RobbieTheWagner RobbieTheWagner changed the title add index to 'show' and 'cancel' events, added 'forward' prop to show… Add index to 'show' and 'cancel' events Jul 22, 2019
@RobbieTheWagner RobbieTheWagner merged commit f943799 into shipshapecode:master Jul 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants