phetsims / mean-share-and-balance

"Mean: Share and Balance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 1 forks source link

Do we actually want to use Dialog for the Mean Info? #230

Closed marlitas closed 1 month ago

marlitas commented 2 months ago

While working on a comment from https://github.com/phetsims/mean-share-and-balance/issues/196, I discovered that 1 year ago Marla had implemented the Dialog API incorrectly. This is why we can see strange things in the studio tree where the dialog has both a visibleProperty and isShowingProperty:

image

It is not an expected part of Dialog's API to use a visibleProperty to toggle on and off the appearance of the dialog.

This incorrect implementation allowed us to do things that are not normally part of dialog UX. For example normally when a dialog pops up in our sims, the background darkens, and as soon as you click anywhere outside of the dialog the dialog immediately goes away.

Dialog Implementation:

image

Current Implementation:

image

This discovery is unfortunate because @amanda-phet pointed out that during interviews a student was exploring moving the balls around while seeing the calculations change in real time.

In the end this is a design decision. We can keep the current behavior we have, and I move away from using a Dialog and instead it will act as a Node that is in front of everything in the notepad, or we use the Dialog API as is intended, and lose some of the current behavior, but remain more consistent with PhET dialog UX.

This should be discussed synchronously to avoid confusion.

amanda-phet commented 1 month ago

Design team wants this to be a non-modal dialog! Or at least a non-modal viewable thing.

Questions:

@zepumph will take it from here.

zepumph commented 1 month ago

I created https://github.com/phetsims/sun/issues/882 and will report back with a recommendation on how to proceed in this sim.

zepumph commented 1 month ago

Today during Dev meeting, it was recommended that we just use a non Dialog node with a close button for this case. The common issue was closed and we can proceed with a custom Node for this context. Thanks!

marlitas commented 1 month ago

This is ready for review by @jbphet.

jbphet commented 1 month ago

Nice. Changes look good, and it seems like a significant improvement. Closing.