search

phetsims / sun

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

Convert AccordionBox to TypeScript #738

Closed samreid closed 2 years ago

samreid commented 2 years ago

For https://github.com/phetsims/center-and-spread/issues/6, it would be nice to have AccordionBox in TypeScript.

samreid commented 2 years ago

I made initial progress on this, but did not fully visit transitive parts like ExpandCollapseButton, and ran into problems around ACCESSIBLE_NAME_BEHAVIOR. @jessegreenberg can you please advise or take a look on ACCESSIBLE_NAME_BEHAVIOR?

zepumph commented 2 years ago

I'm in this file working on https://github.com/phetsims/sun/issues/743 right now. I'll take this on.

zepumph commented 2 years ago

I made various improvements, and some of them bled outside of AccordionBox. I fixed the typing for Behavior functions. @samreid please note an any waiting on Button options,

I also removed AccordionBoxOptions from phet-types. It was a total headache because phet-types wasn't the only spot where options for common code was being declared. It was hard to track down.

@samreid and @chrisklus, I changed code in you sims. Please be aware and let me know if you see any trouble. My main goal was to get things compiling, but I also tried to use the latest TypeScript patterns we have.

@samreid, is there anything else left here?

samreid commented 2 years ago

I skimmed the commits, taking a closer look at the changes in CCK and everything seemed good. There is one TODO listed for this issue remaining:

    // @ts-ignore TODO: private in the same file, but not private in the same class, https://github.com/phetsims/sun/issues/738
    ( node as AccordionBox ).expandCollapseButton.accessibleName = accessibleName;

Some solutions would be:

To me, it makes sense that an AccordionBox's expandCollapseButton would be public. Sound OK?

zepumph commented 2 years ago

Yes, making expandCollapseButton public doesn't bother me in the slightest, but the second approach has been done elsewhere, and it makes the most sense in this case. I don't want to make the button public unless I had a use case that called for it. Feel free to make that happen if you'd like though! I'm ready to close.

samreid commented 2 years ago

That seems great, thanks! Closing.