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

Conversation

@howmanysmall
Copy link
Contributor

@howmanysmall howmanysmall commented Jan 14, 2021

I've made the following changes to the codebase, as seen in the changelog.

  • Fixed several linting problems.
  • Made a few small performance changes (reducing indexing, checking optional values being nil before type checking them, using string.x instead of :x).
  • Made the code better follow the Roblox Lua Style Guide.
  • Changed some of the examples to also follow the Lua style guide better.

Checklist before submitting:

  • Added entry to CHANGELOG.md
  • Added/updated relevant tests
  • Added/updated documentation

Copy link
Contributor

@matthargett matthargett 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! A few minor requests, and then we can review again.

if config.typeChecks then
assert(Type.of(element) == Type.Element, "Expected arg #1 to be of type Element")
assert(renderer.isHostObject(hostParent) or hostParent == nil, "Expected arg #2 to be a host object")
assert(hostParent == nil or renderer.isHostObject(hostParent), "Expected arg #2 to be a host object")
Copy link
Contributor

Choose a reason for hiding this comment

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

if you're changing these a bit, can you also make them say something more specific like:
"Expected the hostParent argument to mountVirtualTree to be a Host Object."?

local TextReverser = Roact.Component:extend("TextReverser")

function TextReverser:init()
self.state = {
Copy link
Contributor

Choose a reason for hiding this comment

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

we may be changing our guidance on this in the future, so leave this one for now.

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.

2 participants