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

should ProjectorModeCheckbox projectorModeEnabledProperty be instrumented? #572

Closed
pixelzoom opened this issue Jul 28, 2019 · 1 comment
Closed
Assignees

Comments

@pixelzoom
Copy link
Contributor

Creating this issue based on feedback by @jessegreenberg in #569 (comment).

There are 2 Properties associated with the "Projector Mode" checkbox that typically appears in the Options dialog for a sim that has this feature.

The first Property is profileNameProperty, the name of the selected ColorProfile. The choices are typically "default" or "projector", and it's presented as a pair of radio buttons in Studio. For example, for Gas Properties:

screenshot_1292

The second Property is internal to ProjectorModeCheckbox, a joist common-code component. It maps profileNameProperty to a boolean value, as required by Checkboxes in general. It currently doesn't appear in Studio because of lack of support for dynamic elements (Options dialog is created on demand). When it does appear, it will look like a typical Checkbox in Studio, featured by default. In Gas Properties, it's phetioID would be:

gasProperties.general.navigationBar.phetButton.phetMenu.optionsDialog.content.projectorModeCheckBox.projectorModeEnabledProperty

The question is whether to instrument projectorModeEnabledProperty. It is sufficient to instrument profileNameProperty, and projectorModeEnabledProperty is an internal implementation detail. If instrumented, it would be as a convenience, at the expense of exposing internals of ProjectorModeCheckbox.

projectorModeEnabledProperty is currently instrumented, because Checkboxes expect their associated Property to be instrumented (so that they can create a linked element). If that's not desired, we'll need to investigate how to get around that -- a good thing to do, since I don't think that's a valid expectation.

Since this came up in the context of Gas Properties PhET-iO design phetsims/gas-properties#30, assigning to @arouinfar.

@zepumph
Copy link
Member

zepumph commented Jul 19, 2021

This was instrumented and design reviewed in #654.

Current implementation:

const projectorModeEnabledProperty = new BooleanProperty( colorProfileProperty.value === ColorProfile.PROJECTOR_COLOR_PROFILE_NAME, {
tandem: options.tandem.createTandem( 'property' )
} );

@zepumph zepumph closed this as completed Jul 19, 2021
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