Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify usage of laserNode.hasKnob #398

Closed
samreid opened this issue Feb 11, 2021 · 3 comments
Closed

Simplify usage of laserNode.hasKnob #398

samreid opened this issue Feb 11, 2021 · 3 comments

Comments

@samreid
Copy link
Member

samreid commented Feb 11, 2021

From #397 (comment) @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
Copy link

@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.

@arouinfar arouinfar assigned samreid and unassigned arouinfar Feb 11, 2021
@pixelzoom
Copy link
Contributor

pixelzoom commented Feb 11, 2021

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 #398 (comment). That will save the next person (or me again, this isn't my first time scratching my head over this) some time.

@samreid samreid added this to the PhET-iO Instrumentation milestone Feb 26, 2021
@samreid samreid removed their assignment Jul 3, 2021
samreid added a commit that referenced this issue Sep 9, 2022
@samreid
Copy link
Member Author

samreid commented Sep 9, 2022

I added the recommended documentation, closing.

@samreid samreid closed this as completed Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants