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

Shouldn't there be a limit to the number of 10 cards that can be used? #25

Closed
Nancy-Salpepi opened this issue Mar 15, 2023 · 6 comments
Closed
Assignees
Labels

Comments

@Nancy-Salpepi
Copy link

Test device
MacBook Air M1 chip

Operating System
13.2.1

Browser
Safari

Problem description
For phetsims/qa#917, on the lab screen, there doesn't seem to be a limit to the number of 10 cards I can put in the play area. Shouldn't there be one?

Visuals
Screenshot 2023-03-15 at 2 55 34 PM

@Nancy-Salpepi Nancy-Salpepi added type:bug Something isn't working type:question Further information is requested labels Mar 15, 2023
@chrisklus
Copy link
Contributor

Thanks @Nancy-Salpepi, nice catch! @amanda-phet could you please comment how many ten frames we should allow? Based on the space available above, I'm thinking 10-12 maybe? After commenting, please reassign to @marlitas and @pixelzoom to implement at their convenience, whoever's up for it.

@chrisklus chrisklus assigned amanda-phet and unassigned chrisklus Mar 16, 2023
@marlitas
Copy link
Contributor

Oooh mine!

@amanda-phet
Copy link
Contributor

amanda-phet commented Mar 17, 2023

I agree, 10-12 seems the most reasonable for dev bounds (and I can't imagine a good pedagogical use beyond 10). Let's say 10.

Over to @marlitas to implement this change.

@amanda-phet amanda-phet assigned marlitas and unassigned amanda-phet Mar 17, 2023
marlitas added a commit to phetsims/number-suite-common that referenced this issue Mar 20, 2023
marlitas added a commit to phetsims/number-suite-common that referenced this issue Mar 20, 2023
@marlitas
Copy link
Contributor

Addressed in the above commits. I started by just eliminating pickability when we reached 10 Ten Frames. However this behavior seemed a bit confusing and remembered some other use cases where the draggable icon no longer appeared as though to indicate the stack ran out. I created a commit for that option as well.

Below are two videos showing both options. If the first option is preferred it would be very easy to revert the second commit where we toggle the icon's visibility.

Option 1: Limit pickability

Screen.Recording.2023-03-20.at.10.25.41.AM.mov

Option 2: Limit visiblity

Screen.Recording.2023-03-20.at.10.24.23.AM.mov

@amanda-phet & @chrisklus let me know your preference.

@chrisklus
Copy link
Contributor

@marlitas and I paired on this. We decided to keep the visibility implementation because the rest of the items on this screen follow the same pattern. We also refactored TenFrameNode.getTenFramePath to take PathOptions so we could simplify the implementation by passing in a visibleProperty option that is a DerivedProperty. We think this is working as expected.

@Nancy-Salpepi can you please confirm on phettest? Feel free to close if looking good.

@chrisklus chrisklus removed the type:question Further information is requested label Mar 20, 2023
@Nancy-Salpepi
Copy link
Author

Looks good! There are now 10 Ten Frames. The toolbox looks empty when all ten are in the play area and the icon reappears when one is returned.

Closing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants