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

Expose Tether event system as imperative and props API #30

Merged
merged 3 commits into from Aug 22, 2016

Conversation

Projects
None yet
2 participants
@slorber
Contributor

slorber commented Aug 15, 2016

handles
#28
#29

@@ -75,18 +78,43 @@ class TetherComponent extends Component {
componentWillUnmount() {
this._destroy()
}
getTether() {

This comment has been minimized.

@souporserious

souporserious Aug 16, 2016

Collaborator

how do you feel about calling this getTetherInstance?

This comment has been minimized.

@slorber

slorber Aug 17, 2016

Contributor

yes, was thinking of using that name too

disable() {
this._tether.disable()
}
enable() {
this._tether.enable()
}
on(event, handler, ctx) {
this._tether.on(event,handler,ctx);

This comment has been minimized.

@souporserious

souporserious Aug 16, 2016

Collaborator

nitpicking, but can we can get spaces between the arguments here as well as the ones below

position() {
this._tether.position()
}
_registerEventListeners() {
if ( this.props.onUpdate ) {

This comment has been minimized.

@souporserious

souporserious Aug 16, 2016

Collaborator

another nitpick, but can we please get the spaces inside the parens removed from here as well as the other to match the original code style.

@slorber

This comment has been minimized.

Contributor

slorber commented Aug 17, 2016

I've made the style changes and renamed getTether.

(didn't run npm install before commit, i guess you'll want to run it yourself before releasing)

@slorber

This comment has been minimized.

Contributor

slorber commented Aug 22, 2016

@souporserious tell me if you want to change anything else :)

@souporserious

This comment has been minimized.

Collaborator

souporserious commented Aug 22, 2016

Thanks for cleaning that up!

@souporserious souporserious merged commit c489c30 into danreeves:master Aug 22, 2016

@slorber

This comment has been minimized.

Contributor

slorber commented Aug 22, 2016

(don't forget to run npm install before release :))

@slorber

This comment has been minimized.

Contributor

slorber commented Aug 25, 2016

@souporserious can you publish the 0.5.3 release plz ? :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment