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

issues with "Out Of Balls!" dialog #72

Closed
pixelzoom opened this issue Aug 30, 2016 · 14 comments
Closed

issues with "Out Of Balls!" dialog #72

pixelzoom opened this issue Aug 30, 2016 · 14 comments

Comments

@pixelzoom
Copy link
Contributor

In the Lab screen, when you reach a maximum number (currently 9999) of balls in any one bin, this ugly dialog pops up:

screenshot_109

And it fails stringTest=long:

screenshot_110

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 30, 2016

@amanda-phet If you have any requests for how you'd like this dialog to look, let me know. I'm going to take a stab at it. Nothing fancy, I'll just make it look respectable and handle i18n properly.

@pixelzoom
Copy link
Contributor Author

I find it a little odd that this dialog is triggered by some maximum number of balls being reached in 1 bin, versus some maximum number of balls being dispensed from the hopper. Since it's such a large number, maybe not worth worrying about. But @amanda-phet and @ariel-phet let me know if you have an opinion on this aspect of it.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 30, 2016

Also wondering why the Play button is still enabled if we're "out of balls"? If I close the dialog and press the Play button, the dialog immediate pops up again. Shouldn't the Play button be disabled until either the eraser button or Reset All button is pressed?

@pixelzoom
Copy link
Contributor Author

Revised dialog, ditched the multiline text (harder to read), decreased font size, increased margins:

screenshot_02

stringTest=long:

screenshot_03

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 30, 2016

Hmmm... This dialog appears only in the Lab screen, where the max is 9999 balls (unlikely to be reached). If doesn't appear in the Intro screen, where the max is 100 balls (likely to be reached), and the Play button is simply disabled when the max is reached.

@amanda-phet Why does one screen need this dialog, while the other does not?

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Aug 30, 2016

Not sure why this is done in LabScreenView when the dialog is displayed:

157 // sets the play button to active.
158 playPanel.setPlayButtonVisible();

Why would we want to make the Play button active when we've reached the max?

@pixelzoom
Copy link
Contributor Author

The Play button now disables when the max number of balls is reached, and is enabled only after pressing eraser or Reset All button (similar to Intro screen).

@pixelzoom
Copy link
Contributor Author

So... The remaining design questions are:

  1. What is the limit that we should be looking for in the Lab screen? In the Intro screen, it's the number of balls dispensed (100). In the Lab screen, it's quite different for some reason - it's the number of balls in any one bin (9999).
  2. Do we even need this dialog? Or is disabling the Play button (as we do in the Intro screen) a sufficient cue?
  3. If we do need this dialog, why doesn't the Intro screen have the same dialog?
  4. Is the revised Dialog acceptable? (screenshot in issues with "Out Of Balls!" dialog #72 (comment))

@pixelzoom pixelzoom changed the title "Out Of Balls" dialog is ugly and fails stringTest issues with "Out Of Balls" dialog Aug 30, 2016
@pixelzoom pixelzoom changed the title issues with "Out Of Balls" dialog issues with "Out Of Balls!" dialog Aug 30, 2016
@pixelzoom
Copy link
Contributor Author

I discussed questions 1, 2 and 3 above with @ariel-phet. All of those had been discussed in design meetings, and the current implementation is what is desired.

So @amanda-phet, all I need is the answer to 4: Is the revised Dialog acceptable? ... as shown in #72 (comment).

@amanda-phet
Copy link
Contributor

  1. Yes. The smaller text is a nice improvement.

@pixelzoom
Copy link
Contributor Author

Thanks @amanda-phet. Closing.

@jessegreenberg
Copy link
Contributor

In the above commit I removed a workaround that was necessary because of phetsims/joist#346. @pixelzoom wanted to let you know in case you want to review. I removed the Panel but kept the spacing the same to match the look of the deployed version.

@pixelzoom
Copy link
Contributor Author

Since this sim is likely to be published before I return from vacation, it's probably best to have @andrea-phet or @jbphet review the changes.

@pixelzoom pixelzoom assigned jbphet and andrealin and unassigned pixelzoom Aug 9, 2017
@andrealin
Copy link
Contributor

Looks good, closing.

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

No branches or pull requests

6 participants