search

AmaranthineCodices / roact-material

Material design in Roblox w/ Roact!
MIT License
20 stars 5 forks source link

Make input event listeners optional #23

Closed ZachCurtis closed 6 years ago

ZachCurtis commented 6 years ago

Not listening for either onClicked or onChecked throws nil value errors. Listeners should be optional and not halt execution.

Double commits because roact-material is using spaces instead of tabs.

ZachCurtis commented 6 years ago

Didn't mean to add the animation edits to my main branch until they were fully accurate with the style ruling; but they don't warrant a revert of this pr.

AmaranthineCodices commented 6 years ago

These listeners are required because there is no purpose to not supplying them. The selection components are not stateful - clicking a checkbox without a listener attached does nothing. It does not toggle the checkbox! In an earlier version, it did, but I removed that because without a listener there's no way to actually get the checkbox's state out.

The correct way to handle these changes is in the parent component, as can be seen in the Checkbox example.

I am going to insist that the animation changes be pulled out and placed in a separate PR.

AmaranthineCodices commented 6 years ago

After thinking further on this, this is not a PR I want to merge due to the remarks I mentioned above. Thanks for the contribution, but these listeners are required to prevent potential bugs!