-
Notifications
You must be signed in to change notification settings - Fork 141
Conversation
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.
Implementation LGTM, but holding off on merging until @LPGhatguy and @ZoteTheMighty have reviewed as well.
One thought is that we probably want to document this.
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.
Oooh, this is such a cool piece of functionality.
Roact 1.1, here I come. I'll still want @ZoteTheMighty to take a look since he's the one who wrote a lot of this code.
A bug came to my attention while working on refactoring as per lpg's review. |
Everything should be working well now! Excuse any stylistic issues as I'm tired, however there shouldn't be many (or any at all). |
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.
Thanks for the contribution!
Looked everything over and it seems like all of my concerns were addressed by previous comments. LGTM!
One thing I forgot about is changelog/documentation (as @AmaranthineCodices mentioned). We'll want an API reference entry at least. That brings up the question of versions, though, so we probably need to figure out how we want to operate going forward, if we want to have something like a development branch. We probably shouldn't add new functionality to master without incrementing the version. @LPGhatguy what do you think we should do about this for now? |
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.
One change to the type check and then I think we're good.
@ZoteTheMighty I think we'll probably operate like other projects do -- keep I'd like to mess with this PR a little bit to see if it makes sense to split our different kinds of bindings apart now, and just keep a common interface. |
After talking on Discord and doing some hacking, I think we're going to close this in favor of #208. |
Implements a function that creates a new binding, which updates whenever any of the bindings passed (via table) updates. Internally as
Binding.join
; public API isRoact.joinBindings
.Example usage: