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

Many sims are using Node.visibleProperty option wrong #1090

Closed
zepumph opened this issue Oct 14, 2020 · 5 comments
Closed

Many sims are using Node.visibleProperty option wrong #1090

zepumph opened this issue Oct 14, 2020 · 5 comments
Assignees

Comments

@zepumph
Copy link
Member

zepumph commented Oct 14, 2020

There are a lot of atypical usages of visibleProperty of Nodes on the project, no doubt because we have only allowed passing in a Property for 6 months, and it was a quiet release of the feature at that.

I will go around and clean up some usages. This work is from #1046

@zepumph zepumph self-assigned this Oct 14, 2020
@zepumph
Copy link
Member Author

zepumph commented Oct 14, 2020

I feel like "wrong" is the wrong word here. In most cases it is just not idiomatic to our normal options pattern, mostly because subtypes have been traditionally handling this logic.

@zepumph
Copy link
Member Author

zepumph commented Oct 14, 2020

  • I see 20 usages of visibleProperty.linkAttribute( this, in the project. Likely all of these can be refactored to use Node.visibleProperty now.

@zepumph
Copy link
Member Author

zepumph commented Oct 21, 2020

Tagging #1047 as partially the base work here.

@zepumph
Copy link
Member Author

zepumph commented Oct 21, 2020

I see 20 usages of visibleProperty.linkAttribute( this, in the project. Likely all of these can be refactored to use Node.visibleProperty now.

I starting going through these, and immediately felt like this was a waste of time. It was so hard to know how to test each occurrence, and the benefits seem negligible. I'm going to close this issue.

@zepumph zepumph closed this as completed Oct 21, 2020
@zepumph
Copy link
Member Author

zepumph commented Oct 21, 2020

I'll add that I also looked at usages of visibleProperty = in the project matching words and case, and didn't see anything else that was suspect. Most were in the model, or in the view but either a DerivedProperty or a non-standard usage.

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

1 participant