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
Custom Event handling methods as solution to circular dependencies #1091
Conversation
… types and annotations
Size Change: +3.09 kB (11%) Total Size: 27.3 kB
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this idea but I believe we should still support the introjs(...).oncomplete(fn => ...)
notion as well (in addition to introjs(...).of('complete', fn => ...)
. I don't really want to introduce a massive backward incompatibility. But I can definitely see that this approach can simplify the codebase.
@@ -34,15 +32,6 @@ export default function exitIntro(targetElement, force) { | |||
forEach(overlayLayers, (overlayLayer) => removeChild(overlayLayer)); | |||
} | |||
|
|||
//remove all helper layers | |||
const helperLayer = targetElement.querySelector(".introjs-helperLayer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why we don't need this anymore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed in another file
@@ -55,15 +44,13 @@ export default function exitIntro(targetElement, force) { | |||
|
|||
removeShowElement(); | |||
|
|||
//clean listeners | |||
DOMEvent.off(window, "keydown", onKeyDown, this, true); | |||
DOMEvent.off(window, "resize", onResize, this, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we handling these somewhere else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh nvm, just saw the off
handler. nice
DOMEvent.off(window, "keydown", onKeyDown, this, true); | ||
DOMEvent.off(window, "resize", onResize, this, true); | ||
// signal to all listeners that introjs has exited | ||
this.fire('exit'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
I'll take a look at |
Just noticing that exitIntro is always called after the oncomplete: if (
this._introItems.length - 1 === this._currentStep &&
typeof this._introCompleteCallback === "function"
) {
this._introCompleteCallback.call(this);
}
exitIntro.call(this, this._targetElement); maybe we can just put that at the top of exitIntro instead |
Sorry no, I sort of assumed the long term plan is to replace the new event handle (
That makes sense @bozdoz. |
I was wrong about this. It's called if skip button is pressed on the last tip, and if nextStep is called when there is no nextStep. Seems fair to keep it. |
Add events to deal with circular dependencies; added some type annotations and definitions; cleaned up some methods.
Also, events could make for a nicer/more flexible API, going forward.
Think of this line:
Devs can add all the introJs.on('exit') handlers they want.