Add error callback for showStep (Issue #274) #291
Conversation
Calling showStep without a proper step target fails, but the tour error callback is not invoked. This change fixes that, and sets the current step number to the step that failed for debugging purposes
|
1 test failed. =( Lemme take a look and see what it's complaining about... |
| @@ -1945,6 +1945,8 @@ | |||
| this.showStep = function(stepNum) { | |||
| var step = currTour.steps[stepNum]; | |||
| if(!utils.getStepTarget(step)) { | |||
| currStepNum = stepNum; | |||
There was a problem hiding this comment.
Let's remove this. IMO, if the step's target doesn't exist, we should leave the tour where it is. This also is probably the source of the build failure.
|
Gah! Sorry for the bad PR. Can definitely remove that bit, but will be slightly less clear if the callback is returning the failed stepNum. Not a deal-breaker though. Shall I refactor and resubmit? I'm not sure what the protocol is here. Thanks for taking a look! |
|
Hmm, fair point. So, maybe it would make sense to optimistically set You can leave this PR open. Go ahead and push a new commit to the feature branch you have open at present (or, in this case, |
Not sure if this will work
just a test to see what's breaking
| @@ -2430,8 +2431,90 @@ | |||
| // Template includes, placed inside a closure to ensure we don't | |||
| // end up declaring our shim globally. | |||
| (function(){ | |||
| // @@include('../../src/tl/_template_headers.js') // | |||
| // @@include('../../tmp/js/hopscotch_templates.js') // | |||
| var _ = {}; | |||
There was a problem hiding this comment.
This shouldn't be included in /src. This code gets added in when running grunt. If you need the output from grunt for testing purposes, the built files will be placed in the tmp/ folder.
This reverts commit 8dfed46.
|
Yep, sorry about that- still getting used to some of this. Am trying to test now, had to install node etc. but now I understand why I was getting the error in my codepen example. |
temporarily set currStepNum then revert
|
Ok, checked this back in here; all tests passing for me locally, and my codepen example is working correctly. Thanks! |
| @@ -1945,6 +1945,10 @@ | |||
| this.showStep = function(stepNum) { | |||
| var step = currTour.steps[stepNum]; | |||
| if(!utils.getStepTarget(step)) { | |||
| var prevStepNum = currStepNum; | |||
There was a problem hiding this comment.
Nitpick... hoist the var declaration to the top of the function.
|
Cool, this PR is looking good to me. Just commented with one minor nitpick... once that's fixed, I'll mark this as Ready to Merge. I'll pull this in tonight or tomorrow. Thanks! |
|
No problem! I should have thought of that; too focused on making sure I built it properly to observe good code style. Thanks for catching that. |
Calling showStep without a proper step target fails, but the tour error
callback is not invoked. This change fixes that, and sets the current
step number to the step that failed for debugging purposes
I'm getting a different error in my codepen example (used in the issue report) related to the 'bubble_default' template, so I wasn't able to test this. I do have this fix in a client code-base however, and that is working fine.
This may be imperfect, if so I apologize. Just thought I'd try to get this in for the maintenance release if possible.