skipUndefinedTrip option should allow function as an alternative to Boolean

[71FIL]

An undefined trip is most often likely to be a problem in an application. Allowing skipUndefinedTrip option to be a function (like canGoPrev/canGoNext), would allow the application to take action upon a trip error (e.g. reporting an error to a back-end server).

[EragonJ]

Hmm my original thought of this option is to help developers when they are trying to develop their trips at the first time. For trips, in my mind (please correct me if I am wrong), these guided tours should be well defined before presenting to users, so in production environment, there should not be any undefined trips.

[71FIL]

Many things can happen in production. Although we'd like to make our tours always right, there might be unforeseen situations that occur in production. The trips we implement rely on dynamic data that might not come back as expected. Someone else might be doing the testing in the organization. The application could be instrumented with handlers that report all errors in the same manner rather than in the console.

My colleague who suggested that change is currently in the process of implementing about 30 different tours. He also has implemented a mode where tours run through automatically with no delay and it would be useful if all errors were to be reported to the application - the handler could record the problem but let the tour proceed by returning true.

[EragonJ]

Hmm ok, I think your point is also right. In order to achieve this, I am thinking about the new way to transfer data. Take this discussion for example :

var trip = new Trip();

trip.on('start', function() {
  // like original onStart
});

trip.on('end', function() {
  // like original onEnd
});

trip.on('error', function(tripIndex, tripObject) {
 // 1. trip is not valid
 // 2. in the future we can also report any error from Trip.js itself, and this cb will always be called.
});

I think this should be the first step to move some callbacks out to this event based style (mainly for global-used things), because I have noticed that it's a little bit confusing and hard to read if I put too many things into each tripObject.

What do you think about this change ?

My colleague who suggested that change is currently in the process of implementing about 30 different tours. He also has implemented a mode where tours run through automatically with no delay and it would be useful if all errors were to be reported to the application - the handler could record the problem but let the tour proceed by returning true.

Can you share more details behind this ? What's handler here ?

[71FIL]

The handler I'm thinking about is a function which would do whatever it wants but return false (i.e. !skip) if trip is to proceed to the following tripObject.

I agree that setting callbacks for individual tripObject can get confusing but in this case we're talking about a global setting. Also, the trip options (with the callbacks) are sent along with the new Trip() call and the same options can be used for all the trips in the application. Having event handlers being set every time a new Trip is instantiated seems tedious in the application.

The more general error callback is a good idea although I would follow the current pattern and add a onError setting which could default to TripUtils.log()

I don't have a strong opinion about this. I would keep in mind that there might be multiple trip instantiations.

[EragonJ]

I agree that setting callbacks for individual tripObject can get confusing but in this case we're talking about a global setting. Also, the trip options (with the callbacks) are sent along with the new Trip() call and the same options can be used for all the trips in the application. Having event handlers being set every time a new Trip is instantiated seems tedious in the application.

Yes i know what you mean, if it's just a global property called onError, then these properties can easily be shared across all trip instances and you can even easily use that as base to customize for certain trips.

Maybe your use case is more complicated than normal users because there may be tons of trips that will be interacted across pages (sounds interesting).

The more general error callback is a good idea although I would follow the current pattern and add a onError setting which could default to TripUtils.log()

No matter how, I agree with this part, instead of passing noop function to it, this default function makes more sense.

Ok, I think we can follow our current pattern first and see how things going later and change it if needed !