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

Checkbox cleanup #488

Closed
6 tasks done
pixelzoom opened this issue Mar 28, 2019 · 5 comments
Closed
6 tasks done

Checkbox cleanup #488

pixelzoom opened this issue Mar 28, 2019 · 5 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Mar 28, 2019

Noticed while working on enabledProperty in another UI component.

  • this.content and this.ownsEnabledProperty do not need to be instance fields. They are used only in the constructor. Convert them to local variables.

  • this.enabledProperty has no visibility annotation. Typically @public in other UI components.

  • Multiple lines are longer than 120 chars.

  • Checkbox doesn't have a demo/test in the sun demo app.

  • options checkboxAppearanceStrategy and contentAppearanceStrategy are unused. Other UI components have been fine with using opacity to indicate disabled, and other UI components typically have disabledOpacity. So switch to this in anticipation of boilerplate being replaced with a mixin or trait.

  • Use SunConstants.DISABLE_OPACITY, rather than duplicating it.

@pixelzoom pixelzoom self-assigned this Mar 28, 2019
@pixelzoom pixelzoom changed the title Checkbox issues Checkbox cleanup Mar 28, 2019
@pixelzoom
Copy link
Contributor Author

Transferring this issue from scenery-phet to sun, since Checkbox lives in sun.

@pixelzoom pixelzoom transferred this issue from phetsims/scenery-phet Mar 28, 2019
pixelzoom added a commit that referenced this issue Mar 28, 2019
pixelzoom added a commit that referenced this issue Mar 28, 2019
pixelzoom added a commit that referenced this issue Mar 28, 2019
…disabledOpacity, fix enabledPropertyListener, #488
@pixelzoom
Copy link
Contributor Author

Work completed in the above commits. @chrisklus randomly selected to review.

@pixelzoom
Copy link
Contributor Author

It's been a long time since this was assigned for review. @ariel-phet please prioritize.

@chrisklus
Copy link
Contributor

Dang, sorry @pixelzoom. Review complete, changes look straightforward and good to go. Back to @pixelzoom to close.

@chrisklus chrisklus assigned pixelzoom and unassigned ariel-phet and chrisklus Aug 8, 2019
@pixelzoom
Copy link
Contributor Author

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants