phetsims / joist

Joist is the main framework for PhET Interactive Simulations. Joist creates and displays the simulation content, home screen, navigation bar, About dialog, enables switching between tabs, and other framework-related features.
MIT License
8 stars 6 forks source link

Add a small UI shift when running the sim in PhET-iO brand with assertions enabled #802

Closed zepumph closed 2 years ago

zepumph commented 2 years ago

From PhET-iO meeting today over in https://github.com/phetsims/studio/issues/253#issuecomment-1099873680.

@arouinfar, can you look into a couple of mockups for us?

I also want to note that right now it may make sense to run this off of if assertions are enabled or not, but in the future it may be best to have a more central point of "We are in PhET-iO and we are in the debug/development mode." I think that may be added in https://github.com/phetsims/studio/issues/253, so we will need to check on that.

arouinfar commented 2 years ago

Here are a few ideas:

image

My preference would be to change the color of the menu dots. It's clearly visible and doesn't shift the layout or alter the branding. Tagging to for review in design meeting.

zepumph commented 2 years ago

I also really like the kabob menu icon change. Want to mock up that same color with opposite colors (like projector mode) to make sure things have enough contrast for both? Or do we need two different colors?

image

zepumph commented 2 years ago

In my opinion it would be a mistake to change the logo. It would be a fair bit of work, and would likely end up being confusing to our branding and as a communication device.

arouinfar commented 2 years ago

In my opinion it would be a mistake to change the logo.

I totally agree. It's a bad idea from a branding perspective and likely more technically difficult to implement.

Want to mock up that same color with opposite colors (like projector mode) to make sure things have enough contrast for both? Or do we need two different colors?

Good thought! I used WebAim's contrast checker and rgb(255, 0, 0) and rgb(255, 85, 0) (PhetColorScheme.RED_COLORBLIND) both pass against black and white backgrounds.

image
zepumph commented 2 years ago

What would it take to "flip the switch" and implement this coloring when assertions ("PhET-iO debug mode") are enabled.

arouinfar commented 2 years ago

Discussed in 4/21/22 design meeting:

@zepumph mocked this up, and the team was happy with this direction.

image
zepumph commented 2 years ago
```diff Index: js/PhetButton.ts IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/js/PhetButton.ts b/js/PhetButton.ts --- a/js/PhetButton.ts (revision 8a034748ec31602ae85bb2c618277bc7cd905e37) +++ b/js/PhetButton.ts (date 1650580998458) @@ -11,9 +11,7 @@ import IReadOnlyProperty from '../../axon/js/IReadOnlyProperty.js'; import Property from '../../axon/js/Property.js'; import Bounds2 from '../../dot/js/Bounds2.js'; -import { AriaHasPopUpMutator, Color } from '../../scenery/js/imports.js'; -import { Image } from '../../scenery/js/imports.js'; -import { Node } from '../../scenery/js/imports.js'; +import { AriaHasPopUpMutator, Color, Image, Line, Node, VBox } from '../../scenery/js/imports.js'; import pushButtonSoundPlayer from '../../tambo/js/shared-sound-players/pushButtonSoundPlayer.js'; import Tandem from '../../tandem/js/Tandem.js'; import IOType from '../../tandem/js/types/IOType.js'; @@ -81,7 +79,15 @@ } ); // The icon combines the PhET logo and the menu icon - const icon = new Node( { children: [ logoImage, menuIcon ] } ); + const icon = new Node( { + children: [ new VBox( { + align: 'bottom', + spacing: 1, + children: [ logoImage, new Line( 0, 0, logoImage.width, 0, { + stroke: 'red', lineWidth: 2 + } ) ] + } ), menuIcon ] + } ); super( icon, backgroundFillProperty, tandem, { highlightExtensionWidth: 6, ```
samreid commented 2 years ago

In today's meeting, @kathy-phet and @arouinfar surmised that this indicator would display in studio since studio will be running in debug mode. @zepumph is that what you were thinking?

samreid commented 2 years ago

@kathy-phet said this would be good to implement for next week. @arouinfar and/or @kathy-phet also indicated it doesn't need to look pixel perfect since it only displays in the debug mode. Self-assigning.

samreid commented 2 years ago

I added a first draft of the proposal. Some notes:

Questions for @arouinfar

Questions for @zepumph

Note to self:

Stating some concerns for the record, even though there was consensus in https://github.com/phetsims/joist/issues/802#issuecomment-1105786270

Also marking as blocks-publication until review is complete.

kathy-phet commented 2 years ago

In today's meeting, @kathy-phet and @arouinfar surmised that this indicator would display in studio since studio will be running in debug mode. @zepumph is that what you were thinking?

I recall that Amy and I did discuss this with MK when we were deciding what design would be, and we all agreed it would be fine. So I don't think this will be new to him when he is back.

arouinfar commented 2 years ago

Is this design acceptable for this week? Any design changes necessary for our May 10 deadline? Please test in phettest.

Here's what it currently looks like on phettest:

image

@samreid I think this looks fine for client training, but I'll go ahead and tag for design meeting to discuss the other issues you've raised.

zepumph commented 2 years ago

I'm not sure if this was discussed last week, but I will say that in studio when the sim is in an iframe, I can only barely see this. Perhaps if it was a bit thicker and just on top of the bottom portion of the log? I'm not sure what is best.

zepumph commented 2 years ago

The design team liked these tweaks today. I'll do it!


Index: js/PhetButton.ts
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/js/PhetButton.ts b/js/PhetButton.ts
--- a/js/PhetButton.ts  (revision 13b40c29f126e68383bd7f510168880a5e8dcd80)
+++ b/js/PhetButton.ts  (date 1652391603537)
@@ -82,13 +82,11 @@
       [

         // The underline in phet-io phetioDebug mode
-        new Line( 0, 0, logoImage.width, 0, {
-          stroke: 'red', lineWidth: 0.6,
-          centerX: logoImage.centerX,
+        new Line( 0, 0, logoImage.width - 7, 0, {
+          stroke: 'red', lineWidth: 3,
+          left: logoImage.left,
           bottom: logoImage.bottom
-        } ),
-
-        logoImage, menuIcon ] :
+        } ), logoImage, menuIcon ] :
       [ logoImage, menuIcon ];

     // The icon combines the PhET logo and the menu icon
zepumph commented 2 years ago

Here is the patch applied, as we decided in phet-io meeting. Closing.

image

kathy-phet commented 2 years ago

@zepumph - Reopening this to confirm we should have the redline if ?ea is run in standalone PhET-iO sim? I think so, because that is basically running with debug on the PhET-iO sim only. But lets check in at Thursday PhET-iO meeting to make sure we all understand how it criss crosses various combinations, and Amy can write it up well. @arouinfar

zepumph commented 2 years ago

We reviewed this in PhET-iO meeting and decided that there was a bug in the build standalone version. This version doesn't have assertions, but adding ?ea shows the red line still. We also went over all other use cases of phetioDebug and reviewed that they seem good. Also note the problem described in https://github.com/phetsims/studio/issues/269 was a large portion of the confusion. Closing