search

phetsims / bending-light

"Bending Light" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/bending-light
GNU General Public License v3.0
8 stars 8 forks source link

Simplify usage of laserNode.hasKnob #398

Closed samreid closed 1 year ago

samreid commented 3 years ago

From https://github.com/phetsims/bending-light/issues/397#issuecomment-776410279 @pixelzoom said:

Changes looks good, and address my concerns about isTranslationEnabledProperty.

hasKnob is still way too overloaded for my taste. I've figured out that it means:

true - add a knob, make the laser rotate by dragging the knob, translate by dragging the laser body
false - don't add a knob, make the laser rotate by dragging the laser body, don't support translation

If it were me, I would (1) document that LaserNode is always rotatable, (2) always show the knob, since there's no harm in doing so and imo it's confusing that it's removed, and (3) replace hasKnob with isTranslatable:

@param {boolean} isTranslatable
  true: the laser can be translated by dragging it by the laser body, rotated by dragging it by the knob
  false: the laser cannot be translated, and can be rotated by dragging anywhere on the laser

... but up to you whether you want to do anything about hasKnob.

I suspect showing the knob on the "Intro" and "More Tools" screens may look a little cluttered. More importantly, I think it is important to have a visual cue on the laser that makes it look different to indicate that dragging the body will have a different behavior. Perhaps the green arrows address this to some extent, but maybe the knob helps as well?

@arouinfar what do you think is the preferable design?

arouinfar commented 3 years ago

@arouinfar what do you think is the preferable design?

The preferable design is to leave things as they are.

The interaction patterns in Intro/More Tools and Prisms are different. On the Intro/More Tools screen, there is only one way to move the laser -- translating along a fixed circular path. On the Prisms screen, the laser can be translated by grabbing its body or rotated about its center by using the knob.

Adding the knob would muddy things on the Intro and More Tools screens. Users may notice that the knob and body do the same thing, and so the interaction on the Prisms screen would be unexpected. While somewhat less important, the knob is not aesthetically pleasing.

pixelzoom commented 3 years ago

Totally fine with leaving the design the way it is. My main concern was the implementation, and the overloading of option hasKnob. If you want to leave the implementation as is, then at least consider documenting the full (complicated!) semantics of hasKnob, as described in https://github.com/phetsims/bending-light/issues/398#issue-806064768. That will save the next person (or me again, this isn't my first time scratching my head over this) some time.

samreid commented 1 year ago

I added the recommended documentation, closing.