Skip to content
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

Implement attribute support into New function. #199

Merged
merged 17 commits into from
Feb 1, 2023
Merged

Implement attribute support into New function. #199

merged 17 commits into from
Feb 1, 2023

Conversation

krypt102
Copy link
Contributor

@krypt102 krypt102 commented Jan 21, 2023

This PR fixes #1 with a relatively simple API to use.
Attributes can be implemented as follows:

local stateValue = Value("AttributeValue2")

local folder = New "Folder" {
    [Attribute "AttributeName"] = "AttributeValue",
    [Attribute "AttributeName2"] = stateValue
} 

Attribute supports state binding, meaning if you set an attribute to a state value, it will automatically update when that state value changes.

AttributeChanged works similarly to OnChange, where you give it an attribute name, and when that attribute changes, it will run your specified callback.

local child = New "Folder" {
    [Attribute "Test"] = "AttributeOne",
    [AttributeChanged "Test"] = function(newValue)
        print("The attribute changed to", newValue) -- This will run when "Test" changes.
    end
}

I've also added some unit tests for attributes, however they might not be the greatest, so feel free to give any feedback for improvement!
Apologies if this PR isn't the best, I haven't done this before, however I'm open to any suggestions!

@krypt102
Copy link
Contributor Author

Not sure why the unit tests didn't diff with an indent, not too much of a problem though.

src/PubTypes.lua Outdated Show resolved Hide resolved
Copy link
Contributor

@Dionysusnu Dionysusnu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there also be an AttributeOut feature as part of this, equivalent to the regular property Out functionality?

src/Instances/AttributeChange.lua Outdated Show resolved Hide resolved
src/Logging/messages.lua Outdated Show resolved Hide resolved
src/Instances/AttributeChange.lua Outdated Show resolved Hide resolved
src/Instances/Attribute.lua Outdated Show resolved Hide resolved
src/Logging/messages.lua Show resolved Hide resolved
@krypt102
Copy link
Contributor Author

krypt102 commented Jan 21, 2023

Should there also be an AttributeOut feature as part of this, equivalent to the regular property Out functionality?

Actually not a bad idea, it's probably not best having the same issue as before when [Out] didn't exist where you'd need to connect change handlers to update state objects. Will implement!

@krypt102
Copy link
Contributor Author

image
Passed all of my unit tests, not sure if anyone wants to add more but go ahead!

@dphfox
Copy link
Owner

dphfox commented Jan 23, 2023

Thanks for the PR! This won't be included in v0.2 since that's literally about to release, but I'll definitely look into including this alongside the rest of the instance changes planned for v0.3 :)

@krypt102
Copy link
Contributor Author

Thanks for the PR! This won't be included in v0.2 since that's literally about to release, but I'll definitely look into including this alongside the rest of the instance changes planned for v0.3 :)

You're welcome! Hopefully it gets added in v0.3 provided that my code is up to standard 😃

@krypt102 krypt102 marked this pull request as draft January 29, 2023 02:34
@krypt102
Copy link
Contributor Author

Gonna mark this as a draft for now until I sort out the indenting problems!

@krypt102
Copy link
Contributor Author

Finally! I got the indents to work, I messed around with my VSC settings and stopped it from adding spaces, then went through the files and fixed it. If there's still problems please re-review, going to mark ready again.

@krypt102 krypt102 marked this pull request as ready for review January 29, 2023 02:57
@dphfox dphfox added the enhancement New feature or request label Feb 1, 2023
@dphfox dphfox added the ready to work on Enhancements/changes ready to be made label Feb 1, 2023
@dphfox
Copy link
Owner

dphfox commented Feb 1, 2023

Alright, looks good to me. What do you guys think?

@krypt102
Copy link
Contributor Author

krypt102 commented Feb 1, 2023

Looks disgusting to me Nah I think I've put all the features that are necessary, we have [Attribute], [AttributeChange] and [AttributeOut], so it seems good for me!

@dphfox dphfox merged commit 3cde136 into dphfox:main Feb 1, 2023
@krypt102 krypt102 deleted the attribute-support branch February 1, 2023 20:47
@dphfox dphfox mentioned this pull request Feb 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add attributes support to New
4 participants