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

Visibility of fuseRepairButton and batteryReverseButton gets overridden #956

Closed
arouinfar opened this issue Feb 7, 2023 · 3 comments
Closed

Comments

@arouinfar
Copy link
Contributor

In #917 (comment) we discovered that the fuseRepairButton.visibleProperty was read-only and we wanted to make it writable. @matthew-blackman made the change, but it revealed the reason why it was probably read-only in the first place.

The model sets fuseRepairButton.visibleProperty every time a fuse is selected/deselected. Setting the visibleProperty to false will only hide button until the next time a fuse is selected (assuming isRepairableProperty: true).

The same is also true of batteryReverseButton.visibleProperty. Each time a battery with isReversibleProperty: true is selected, the model will set visibleProperty: true.

Interestingly, trashButton.visibleProperty does not exhibit this same behavior. If a client hides the trash button, it stays hidden. For consistency and convenience, it would be nice if we could do the same with fuseRepairButton.visibleProperty and batteryReverseButton.visibleProperty.

Clients could technically use isRepairableProperty and isReversibleProperty to hide these buttons, but doing it for every single fuse and battery that students create is a real drag. It would be simpler to have global control over the button's visibility.

samreid added a commit that referenced this issue Feb 7, 2023
…rolled by the isRepairableProperty and by visibleProperty, see #956
@samreid
Copy link
Member

samreid commented Feb 7, 2023

We added a 2nd gate for the fuse, still need to do the batteries.

@samreid
Copy link
Member

samreid commented Feb 7, 2023

@matthew-blackman and I addressed both cases, thanks for discovering this. Ready for review, and please close if all is well.

@arouinfar
Copy link
Contributor Author

Thanks, looks good!

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