Skip to content
This repository has been archived by the owner on Dec 13, 2023. It is now read-only.

joinBindings implementation #204

Closed
wants to merge 13 commits into from
Closed

joinBindings implementation #204

wants to merge 13 commits into from

Conversation

Reselim
Copy link

@Reselim Reselim commented May 4, 2019

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 is Roact.joinBindings.

Example usage:

local colorBinding = Roact.joinBindings({
	hue = hueBinding,
	saturation = saturationBinding,
	value = valueBinding,
}):map(function(values)
	return Color3.fromHSV(values.hue, values.saturation, values.value)
end)

@coveralls
Copy link

coveralls commented May 4, 2019

Coverage Status

Coverage increased (+0.2%) to 93.226% when pulling a6eaf12 on Reselim:binding-join into 41af74d on Roblox:master.

Copy link
Contributor

@AmaranthineCodices AmaranthineCodices left a 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.

Copy link
Contributor

@LPGhatguy LPGhatguy left a 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.

src/Binding.lua Outdated Show resolved Hide resolved
src/Binding.spec.lua Show resolved Hide resolved
@Reselim
Copy link
Author

Reselim commented May 5, 2019

A bug came to my attention while working on refactoring as per lpg's review. Binding.join hooks up connections before it's subscribed to for the first time; this will be fixed in an upcoming commit.

@Reselim
Copy link
Author

Reselim commented May 5, 2019

Everything should be working well now! Excuse any stylistic issues as I'm tired, however there shouldn't be many (or any at all).

@AmaranthineCodices AmaranthineCodices self-requested a review May 5, 2019 06:50
src/Binding.spec.lua Outdated Show resolved Hide resolved
src/Binding.lua Outdated Show resolved Hide resolved
src/Binding.lua Outdated Show resolved Hide resolved
src/Binding.lua Show resolved Hide resolved
src/Binding.lua Outdated Show resolved Hide resolved
src/Binding.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@ZoteTheMighty ZoteTheMighty left a 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!

@ZoteTheMighty
Copy link
Contributor

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?

Copy link
Contributor

@AmaranthineCodices AmaranthineCodices left a 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.

src/Binding.lua Outdated Show resolved Hide resolved
@LPGhatguy
Copy link
Contributor

@ZoteTheMighty I think we'll probably operate like other projects do -- keep master as our dev branch and only increment the version number when we do releases.

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.

@LPGhatguy
Copy link
Contributor

After talking on Discord and doing some hacking, I think we're going to close this in favor of #208.

@LPGhatguy LPGhatguy closed this May 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants