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

Added a plugin action for the sync in command. #80

Closed
wants to merge 3 commits into from

Conversation

dnurkkala
Copy link

@dnurkkala dnurkkala commented Jun 19, 2018

Closes #79.

@LPGhatguy
Copy link
Contributor

Cool, thanks!

end

toolbar:CreateButton("Sync In", "Sync into Roblox Studio", Config.icons.syncIn).Click:Connect(syncIn)
plugin:CreatePluginAction("RojoSyncIn", "Sync In Rojo Project", "Pulls the connected Rojo project into Studio.").Triggered:Connect(syncIn)
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like this line is too long based on the Luacheck policy we've got, and I'm inclined to agree.

Can we take the strings out and put them into constants? I think the 'Sync In Rojo Project' part should probably be the same between the button and the action.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.667% when pulling b62f345 on dnurkkala:v0.4.x into fb7bfa9 on LPGhatguy:v0.4.x.

end

local dShort = "Sync In"
local dLong = "Sync into Roblox Studio"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we spell out these variable names? shortLabel and longLabel would be fine I imagine.

@LPGhatguy
Copy link
Contributor

I tried to manually merge this locally and messed it up!

Rest assured, your code has landed and should be in the next release.

@LPGhatguy LPGhatguy closed this Jun 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants