-
Notifications
You must be signed in to change notification settings - Fork 6.7k
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
Common libraries in src/lib #625
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,34 +31,43 @@ | |
recordStartingState( rootId ); | ||
|
||
// LIBRARY FUNCTIONS | ||
// Below are definitions of the library functions we return at the end | ||
// Definitions of the library functions we return as an object at the end | ||
|
||
// `pushElement` adds a DOM element to the gc stack | ||
var pushElement = function( element ) { | ||
elementList.push( element ); | ||
}; | ||
|
||
// Convenience wrapper that combines DOM appendChild with gc.pushElement | ||
// `appendChild` is a convenience wrapper that combines DOM appendChild with gc.pushElement | ||
var appendChild = function( parent, element ) { | ||
parent.appendChild( element ); | ||
pushElement( element ); | ||
}; | ||
|
||
// `pushEventListener` adds an event listener to the gc stack | ||
var pushEventListener = function( target, type, listenerFunction ) { | ||
eventListenerList.push( { target:target, type:type, listener:listenerFunction } ); | ||
}; | ||
|
||
// Convenience wrapper that combines DOM addEventListener with gc.pushEventListener | ||
// `addEventListener` combines DOM addEventListener with gc.pushEventListener | ||
var addEventListener = function( target, type, listenerFunction ) { | ||
target.addEventListener( type, listenerFunction ); | ||
pushEventListener( target, type, listenerFunction ); | ||
}; | ||
|
||
// If the above utilities are not enough, plugins can add their own callback function | ||
// to do arbitrary things. | ||
// `addCallback` If the above utilities are not enough, plugins can add their own callback | ||
// function to do arbitrary things. | ||
var addCallback = function( callback ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a bit hard from the name of it to understand what callback is it, why to add it, when it will be called. It took me a while (and reading the code) to realise it's simply a callback called on teardown. So I would suggest renaming it to |
||
callbackList.push( callback ); | ||
}; | ||
addCallback( function( rootId ) { resetStartingState( rootId ); } ); | ||
|
||
// `teardown` will | ||
// - execute all callbacks in LIFO order | ||
// - call `removeChild` on all DOM elements in LIFO order | ||
// - call `removeEventListener` on all event listeners in LIFO order | ||
// The goal of a teardown is to return to the same state that the DOM was before | ||
// `impress().init()` was called. | ||
var teardown = function() { | ||
|
||
// Execute the callbacks in LIFO order | ||
|
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.
Probably a copy/paste leftover here, I'll raise a PR to remove it
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.
#627