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

Rectify slider and controls names #344

Closed
samreid opened this issue Aug 26, 2022 · 8 comments
Closed

Rectify slider and controls names #344

samreid opened this issue Aug 26, 2022 · 8 comments
Assignees

Comments

@samreid
Copy link
Member

samreid commented Aug 26, 2022

phetsims/tandem#267 revealed that there are places where a NumberControl is being instantiated but called a Slider. I renamed the tandem, but the class hierarchies should be renamed to update it as well. I'll add 2 TODOS showing the entry points for the problems.

samreid added a commit that referenced this issue Aug 26, 2022
@marlitas
Copy link
Contributor

Re-assigning to myself for the new character set publication.

@marlitas marlitas assigned marlitas and Luisav1 and unassigned jessegreenberg Oct 25, 2023
@marlitas marlitas removed their assignment Nov 17, 2023
@marlitas
Copy link
Contributor

@samreid The TODO's you highlighted are actually sliders. This is what the documentation in PhysicalSlider.ts says:

This is just a slider with a label and labeled tick marks. It extends
NumberControl to leverage the layout of the label and slider so that it conveniently matches the layout
with other NumberControls used in this sim.

I think the tandems should be renamed to use "slider" instead of numberControl to match the implementation strategy. At the least, the classes should not be renamed, since they clearly describe what is actually being used and displayed for the sim. What are your thoughts here?

@marlitas marlitas assigned samreid and unassigned Luisav1 Nov 17, 2023
@samreid
Copy link
Member Author

samreid commented Nov 17, 2023

PhysicalSlider extends PhysicalNumberControl which extends NumberControl. So it is more of a "has a" slider rather than "is a" slider, since it doesn't extend HSlider, VSlider or Slider. That's why the tandemNameSuffix: 'Control', was in play. I'm not sure what's best. I'm not really sure why the sim wants to use NumberControl to create a slider instead of just creating a slider. Not sure how much time you want to spend here.

@marlitas
Copy link
Contributor

@samreid and I talked and we decided that things are fine as-is. We are closing this issue.

@phet-dev phet-dev reopened this Nov 18, 2023
@phet-dev
Copy link
Contributor

Reopening because there is a TODO marked for this issue.

@samreid samreid assigned marlitas and unassigned samreid Nov 18, 2023
Luisav1 added a commit that referenced this issue Nov 28, 2023
@Luisav1
Copy link
Contributor

Luisav1 commented Nov 28, 2023

Removed the unnecessary TODO left over. Closing.

@Luisav1 Luisav1 closed this as completed Nov 28, 2023
@phet-dev phet-dev reopened this Nov 29, 2023
@phet-dev
Copy link
Contributor

Reopening because there is a TODO marked for this issue.

Luisav1 added a commit that referenced this issue Nov 29, 2023
@Luisav1
Copy link
Contributor

Luisav1 commented Nov 29, 2023

Sorry, missed removing a TODO referencing this issue. Closing.

@Luisav1 Luisav1 closed this as completed Nov 29, 2023
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

5 participants