-
Notifications
You must be signed in to change notification settings - Fork 13
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
CustomDialog has design issues #25
Comments
(1) When the dialog first opens, none of the fields is associated with the keypad, so typing on the keypad and pressing Enter doesn't change anything. (2) Since the dialog closes when you press Enter, you can only change 1 field. If you want to change N fields, you'll need to open the dialog N times. (3) The behavior of the "edit" buttons next to the fields are inconsistent with other PhET sims. In other PhET sims (e.g. unit-rates), pressing an edit button opens a keypad. In this sim, it changes which field is attached to the keypad. (4) There's an "Air Resistance" check box. And if it's the only thing that you want to change, it seems odd that you have to press "Enter" to close the dialog. (5) Consider right justifying the values in their fields. Highly recommended to consult with your design team leader before doing any further implementation work on this dialog. |
@amanda-phet @amyh-phet mentioning you because this dialog has design issues, and you're listed as lead designers in the About credits. |
@arouinfar Any thoughts? |
Sorry @amyh-phet, I should have assigned @arouinfar. |
I recall being able to set up multiple parameters at once, such as for a lab or homework assignment, was important to the design for this sim. Having the keypad on the right of the values seems user-friendly because you can input a number without covering the other values. I think it was important to Mike to have this sort of interaction. I agree with everything in #25 (comment) .
That should be changed as it's not behaving correctly. You should be able to submit a value and then go on to another field.
The keypad could be not visible until the user has clicked an edit button, then it appears. When they submit the value, the keypad disappears until they click a different edit button. |
Another content issue: (6) If the dialog is used to change multiple fields, do the changes take effect as each field is changed, or when the dialog is closed? (Keeping in mind that the dialog will be modal, and other parts of the sim will be dimmed.) It's not immediately obvious to me what the current behavior is, but I suspect that values changes take effect as soon as you press "Enter", while "Air Resistance" takes effect as soon as you toggle the check box. |
@amanda-phet said:
Agreed, this is a more standard dialog approach. If changes take effect as soon as a field or control is changed (which is what I'd recommend), then you don't need an "Accept" button (or similar name). But consider adding a "Close" button, which is equivalent to clicking outside the dialog. |
Note that a close button is apparently necessary to meet accessibility standards. See phetsims/joist#413. If you do add a close button, beware that the close button feature on joist.Dialog has problems. See phetsims/joist#346. |
This custom dialog is basically getting nixed from the design, so I think we can close. |
Related to #22 (high-level review).
Since joist.Dialog is still not ready for prime time (see https://github.com/phetsims/joist/issues/359), you'll unfortunately need to create your own dialog for things like
CustomDialog
.There are a couple of problems with how
CustomDialog
currently functions as a dialog:(1) It needs a way to close it without pressing the "Enter" button.
(2) It should be modal; that is, input to other parts of the sim are blocked while it's visible.
(3) The rest of the sim should appear to "dim" when the dialog opens, similar to what happens when you open the "About" dialog from the PhET menu.
You might consider looking at the implementation of KeypadLayer and KeypadPanel in unit-rates. Press one of the yellow "edit" buttons in that sim to demonstrate.
The text was updated successfully, but these errors were encountered: