search

phetsims / sun

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

evaluate the various "spinner" UI components #436

Open pixelzoom opened 5 years ago

pixelzoom commented 5 years ago

I recently created SCENERY_PHET/FineCoarseSpinner to factor out duplication in Gas Properties and FAMB.

In https://github.com/phetsims/scenery-phet/issues/450#issuecomment-449513041, @samreid pointed out the proliferation of "spinner" UI components.

... I did notice several other *Spinner.js components in our libraries.

AccessibleNumberSpinner FineCoarseSpinner LeftRightNumberSpinner LeftRightSpinner NumberSpinner RoundNumberSpinner UpDownSpinner

I bet we have some opportunity to coalesce these. For instance, the LeftRightSpinner + LeftRightNumberSpinner + FineCoarseSpinner maybe could all use the same type with different options.

To that list I would also add NumberPicker, which is in fact a type of spinner.

Lower hanging fruit is the unnecessary duplication in LeftRightSpinner and UpDownSpinner, identified in https://github.com/phetsims/scenery-phet/issues/311.

We should first evaluate design, then implementation. Does UX really require so many different types of spinners? Which spinners can share implementation?

Assign to @ariel-phet to prioritize and assign. Recommended to start by assigning to a designer.

ariel-phet commented 5 years ago

@pixelzoom my gut instinct would be that we can potentially coalesce some implementation, but we have been (reasonably) good in HTML5 at only adding a new UI component when we felt appropriate. Agreed it would be good to review, to see where we are at, however, probably does not make sense until sometime in February. Leaving assigned to myself and marking deferred for now.

pixelzoom commented 5 years ago

I did some work on NumberSpinner today in https://github.com/phetsims/sun/issues/472. And I discussed spinner a11y with @jessegreenberg.

Some observations relevant to this issue:

(1) NumberSpinner and NumberPicker are both "spinner" type UI components that deal with numeric values. And they both mix in AccessibleNumberSpinner.

(2) NumberSpinner and NumberPicker have surprisingly different APIs. Should their APIs be more similar? How similar? What needs to change? How do the similarities and differences impact a11y and PhET-iO?

(3) NumberSpinner and NumberPicker share no code. Since they are both "number spinners" that seems odd. It also means that we don't reap the potential a11y and phet-io benefits of having a base class.

(4) Having to specify both valuePattern (for NumberSpinner) and a11yValuePattern (for AccessibleNumberSpinner) seems redundant. They are 2 different options to support the use case where description needs to be different than what is displayed. How typical is that use case? Should a11yValuePattern default to valuePattern?

(4) Spinners in general deal with any type of object. Number spinners (generally) are a specialized subclass of spinners. In Equality Explorer, we needed a general object spinner, which has not yet been migrated to common code, see https://github.com/phetsims/scenery-phet/issues/345. When that is needed, it will impact the entire "spinner" hierarchy, including AccessibleNumberSpinner.

(5) NumberSpinner and NumberPicker are currently the only places where AccessibleNumberSpinner is used. If we need to start mixing AccessibleNumberSpinner into other "spinners" identified in https://github.com/phetsims/sun/issues/436#issue-397125835, that's going to be quite a mess.

ariel-phet commented 5 years ago

Noting this may be potentially good work also for hopping into https://github.com/phetsims/tasks/issues/980

arouinfar commented 5 years ago

@ariel-phet has tapped me to take a look at the various flavors of spinners.

@pixelzoom would it be possible for you to list the repos using the various spinner types? I don't have a way to search all of phet code, so I'd need to go into every sim repo and manually dig through .js files, which isn't particularly efficient (and likely inaccurate).

pixelzoom commented 5 years ago

@arouinfar Here's what I could find. There may be more "one off" sim-specific spinners.

sun/NumberSpinner

scenery-phet/NumberPicker

scenery-phet/FineCoarseSpinner

scenery-phet/LeftRightSpinner, UpDownSpinner, see https://github.com/phetsims/scenery-phet/issues/311

Sim-specific "one offs"

In addition to the above UI components, sun/AccessibleNumberSpinner also exists. It is added to UI components to provide accessibility for number spinners. It is currently used in sun/NumberSpinner and scenery-phet/NumberPicker.

arouinfar commented 5 years ago

Thanks @pixelzoom! This list is super helpful.

marlitas commented 2 years ago

I opened up issue: https://github.com/phetsims/scenery-phet/issues/744 stating that it was rather confusing to find NumberPicker in scenery-phet and NumberSpinner in Sun when they are virtually the same component.

Closing that issue as it is a duplicate of this issue, but @samreid and @pixelzoom also concluded that we could at least move NumberPicker into sun so that they are in the same repo.

Self-assigning to get that change going.

marlitas commented 2 years ago

NumberPicker has been moved to sun, and demos have been updated accordingly. Unassigning.