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

Use AXON/EnabledProperty #134

Closed
samreid opened this issue Mar 26, 2024 · 5 comments
Closed

Use AXON/EnabledProperty #134

samreid opened this issue Mar 26, 2024 · 5 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Mar 26, 2024

From the code review #130, one item in the code review checklist says:

When defining a boolean Property to indicate whether something is enabled with a tandem name enabledProperty, use AXON/EnabledProperty. This should be done in both the model and the view. If you're using a DerivedProperty, skip this item.

It looks like this code matches the description and should be changed:

// For PhET-iO. See https://github.com/phetsims/faradays-electromagnetic-lab/issues/105
const enabledProperty = new BooleanProperty( true, {
tandem: options.tandem.createTandem( 'enabledProperty' ),
phetioFeatured: true
} );

@pixelzoom
Copy link
Contributor

I believe that CRC item is bogus. EnabledProperty doc says:

In general this should never be called by clients, but instead is factored out for consistency in PhET libraries.

But let's ask @zepumph, because I seem to recall that he had some 411 on this topic in the past.

@zepumph
Copy link
Member

zepumph commented Mar 27, 2024

I don't have a lot of preference either way. EnabledPropety was basically created just to factor out the tandem name enabledProperty. I think it would be an easy step to apply this to this sim, but also this isn't a hill I'm going to die on. It does seem like either the CRC item or the EnabledProperty doc needs to change though.

EnabledProperty the file was created 10/2020. The CRC item was added in 11/2020. So I'd say part of the initial implementation was the recognition that EnabledProperty could (should?) be used elsewhere. I'll update the EnabledProperty doc. Up to you two about how to proceed on the CRC item and for FEL.

zepumph added a commit to phetsims/axon that referenced this issue Mar 27, 2024
@zepumph zepumph assigned samreid and unassigned zepumph Mar 27, 2024
@zepumph
Copy link
Member

zepumph commented Mar 27, 2024

I really can still see it both ways, but I don't love recommending EnabledProperty given that you still have to hard code 'enabledProperty' as a string, and all the class does is assert that you spelled it right. That makes me sad.

@samreid
Copy link
Member Author

samreid commented Mar 27, 2024

It also brings these defaults:

      phetioDocumentation: 'Determines whether the element is enabled (true) or disabled (false).',
      phetioFeatured: true,

@samreid samreid removed their assignment Mar 27, 2024
@pixelzoom
Copy link
Contributor

Seems like a lot of effort just to verify that the tandem name is "enabledProperty", and the wrong place to be verifying the tandem name. And why EnabledProperty, when we don't have VisibleProperty, InputEnabledProperty, etc.?

That said... I switched to EnabledProperty and gave it more specific phetioDocumentation:

    // For PhET-iO. See https://github.com/phetsims/faradays-electromagnetic-lab/issues/105
    const enabledProperty = new EnabledProperty( true, {
      tandem: options.tandem.createTandem( 'enabledProperty' ),
      phetioDocumentation: 'Determines whether the water faucet is enabled (true) or disabled (false).',
      phetioFeatured: true
    } );

pixelzoom added a commit that referenced this issue Mar 28, 2024
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