phetsims / phet-core

Core utilities used by all PhET simulations.
MIT License
8 stars 6 forks source link

Generalize Orientation #76

Closed samreid closed 4 years ago

samreid commented 5 years ago

Number Line Integers, Circuit Construction Kit and Area Model need an Orientation Enumeration. We discussed on slack:

Sam Reid:house_with_garden: 12:13 PM
Number Line integers needs `new Enumeration( [ ‘HORIZONTAL’, ‘VERTICAL’ ] ) ` and I also need this in scenery-phet capacitor node.  Should I factor it out to somewhere, perhaps phet-core?
What would it be called?
Chris Klusendorf:house_with_garden: 12:18 PM
I see OrientationEnum, which uses cardinal directions instead, so Orientation is available? (even though OrientationEnum  doesn’t follow our convention)
Sam Reid:house_with_garden: 12:19 PM
OrientationEnum seems specific to faradays-law, at least for now.
Jonathan Olson:office: 12:19 PM
There's Orientation (refactorable to common code, for horizontal/vertical)
Direction (somewhat older) was for cardinal directions (North/South/East/West)
Sam Reid:house_with_garden: 12:20 PM
area-model-common Orientation seems very sim-specific
Jonathan Olson:office: 12:20 PM
How is it sim-specific? I don't see anything specific to the sim
Chris Klusendorf:house_with_garden: 12:26 PM
It just looks overboard for an enum, but maybe that’s fine if it works just the same. It doesn’t use Enumeration.js, though, which I thought we were supposed to stick with now. (edited) 
Sam Reid:house_with_garden: 12:36 PM
For Number Line and CCK we just need `new Enumeration( [ ‘HORIZONTAL’, ‘VERTICAL’ ] )` but I can see how it would be nice to just have one thing that does it all, even if not all sims use all features.  JO do you recommend moving area model Orientation to common code for this purpose?
What repo do you want it in?
And since it is not Enumeration, we may need to create its own IO type?
Could area model Orientation be implemented as Enumeration using beforeFreeze?
Jonathan Olson:office: 12:42 PM
Presumably it could technically be implemented, I haven't looked up what we're doing for rich enumerations lately

I'll look into updating the Area Model implementation to use Enumeration, so it will work seamlessly with PhET-iO (using EnumerationIO), then ask @jonathanolson to take a look. If that checks out, we can move this to phet-core and update Number Line and Circuit Construction Kit to use it.

samreid commented 5 years ago

@jonathanolson said:

That style of refactoring looks good to me

I'll work towards moving this to PHET_CORE

samreid commented 5 years ago

OK I moved this to PHET_CORE. Leaving assigned to @jonathanolson to see if there's anything else to do. I'll also create an issue in number line to use this new common code, and I'll start using it in circuit-construction-kit (via scenery-phet).

zepumph commented 4 years ago

Is this ready for production? I created https://github.com/phetsims/sun/issues/596 because it seems like the right thing to do. Please note if we should hold off until this is reviewed.

zepumph commented 4 years ago

To support ariaOrientation, instead of just using the same property as for layout box, I added another one specifically for this purpose. That was we can easily change it if they deviate for some reason. See https://github.com/phetsims/phet-core/commit/7e5dc05564c5a1cb94e30d96a5f70f56a3697451 and let me know if that is problematic.

jonathanolson commented 4 years ago

Everything looks good to me (patched up some JSDoc things in the above commit). Thanks!