search

phetsims / sun

User-interface components for PhET simulations, built on top of Scenery.
MIT License
4 stars 12 forks source link

Slider is not setting `valueProperty` and `enabledRangeProperty` in options #842

Closed pixelzoom closed 1 year ago

pixelzoom commented 1 year ago

Similar to https://github.com/phetsims/scenery-phet/issues/808, and related to https://github.com/phetsims/phet-core/issues/130 ...

Slider is not setting valueProperty and enabledRangeProperty in options, both of which are required by superclass (super trait?) AccessibleSlider.

Assigning to @zepumph and @jessegreenberg to resolve.

pixelzoom commented 1 year ago

You'll need to address this ts-expect-error in Slider.ts:

    // @ts-expect-error see https://github.com/phetsims/sun/issues/842
    const options = optionize<SliderOptions, SelfOptions, ParentOptions>()( {
zepumph commented 1 year ago

They are filled in manually based on other options, which is why they can't be set as "defaults" in the first options block,

https://github.com/phetsims/sun/blob/2bddc7f95561cff8ce6f06aaefbc73a9b465b756/js/Slider.ts#L299-L312

I refactored some bits so that all required args for the super are filled in all at once. This allowed us to get rid of the ts-expect-error.

pixelzoom commented 1 year ago

@jessegreenberg what is your timeframe for reviewing this issue?

jessegreenberg commented 1 year ago

Thanks for doing this @zepumph. I understand now how the factored out RequiredParentOptionsSuppliedBySlider lets us put optional fields in options so that we can fill in the required fields for the super later in superOptions. It looks right to me.

I did not do any regression testing for this.