phetsims / color-vision

"Color Vision" is an educational simulation in HTML5, by PhET Interactive Simulations.
http://phet.colorado.edu/en/simulation/color-vision
GNU General Public License v3.0
1 stars 7 forks source link

screen icon boundaries are not visible on home screen #49

Closed pixelzoom closed 10 years ago

pixelzoom commented 10 years ago

The screen icons have black backgrounds, and they are place on a black home screen - screenshot below. So you can't see the boundaries of the unselected icon. I think it would look better if they had either a non-black background, or some subtle stroke that allows you to see the boundaries. Also note that home screen and navbar can have different icons, so if you like the black background on the navbar icons, they could have different backgrounds on home screen only.

screenshot_343

pixelzoom commented 10 years ago

This looks better to me on the home screen, ColorVisionConstants.HOME_SCREEN_ICON_FILL: 'rgb(220,220,220)'. I would use black background for the navbar icons -- and it looks like you're already creating separate icons for the navbar.

screenshot_344

pixelzoom commented 10 years ago

This is a design issue, might want to consult with design team before making changes.

aaronsamuel137 commented 10 years ago

Not sure what the best call is here. Assigning to @ariel-phet for design input.

ariel-phet commented 10 years ago

I might go with a every so slightly darker color than @pixelzoom chose, but I agree, keep the navbar icons with a black background and go for more of this look on the homescreen.

aaronsamuel137 commented 10 years ago

I have changed the background color to 'rgb(180,180,180)'. See the screen shot:

homescreen

Not sure who should review this.

aaronsamuel137 commented 10 years ago

Assigning to @ariel-phet for review and suggestions.

ariel-phet commented 10 years ago

Hmm, lets go with what @pixelzoom suggested, now that I see it in context, this grey just looks a bit too "disabled". We have a brief team review of color vision this week at design meeting.

samreid commented 10 years ago

Just wanted to throw in a suggestion in advance of the color vision design meeting. The main problem, as @pixelzoom described is:

[...] you can't see the boundaries of the unselected icon.

In my opinion, it looks odd to show the bulbs against a non-black background, since they are against a black background throughout the sim, so how about adding a border to the homescreen icons like so?

border2

With Joist the way it is, it would show a double-border around the selected homescreen icon, which could be odd, but in my opinion, keeping a black background for the bulbs seems like an important point for consistency.

aaronsamuel137 commented 10 years ago

I implemented Sam's idea, just so people can see what it looks like with the double-border. We can change it if needed.

border

ariel-phet commented 10 years ago

Hmm, not sure I dig the double border. I think this idea could work without the double border. I also think the very thin stroke (for the unselected) would be the way to go, if we wanted to keep them black.

aaronsamuel137 commented 10 years ago

OK I have shifted back to @pixelzoom's idea for now because getting rid of the double border would require modifying joist. Perhaps we can bring this up at the next design meeting and determine which way is best here.

samreid commented 10 years ago

I added the border using code in Joist using this code:

      var smallIconContent = new Node( {opacity: 0.5, children: [screen.homeScreenIcon], scale: sim.screens.length === 4 ? HEIGHT / screen.homeScreenIcon.height :
                                                                                                sim.screens.length === 3 ? 1.25 * HEIGHT / screen.homeScreenIcon.height :
                                                                                                sim.screens.length === 2 ? 1.75 * HEIGHT / screen.homeScreenIcon.height :
                                                                                                HEIGHT / screen.homeScreenIcon.height} );
      var smallFrame = new Rectangle( 0, 0, smallIconContent.width, smallIconContent.height, {stroke: 'gray', lineWidth: 0.7} );
      var smallIcon = new Node( {children: [smallFrame, smallIconContent]} );

and it looks like this:

image

To make this work for this sim, we would just need to enable this as an option that is passed to the sim on creation (so it is only on for certain sims), which wouldn't be too tricky.

aaronsamuel137 commented 10 years ago

Great! I'm happy to do this if this is the option we want to go with.

ariel-phet commented 10 years ago

Yes, lets enable this as an option, since we have several sims with a black background scheme.

samreid commented 10 years ago

I added this option and use the option from color vision. Reassigning to @aaronsamuel137 to review my changes above. @aaronsamuel137 and @ariel-phet may also want to review the exact look of the implementation (line color, line stroke width, color vision icon background shade, etc).

aaronsamuel137 commented 10 years ago

Changes look good to me. I am fine to close the issue if the design looks good to @ariel-phet

aaronsamuel137 commented 10 years ago

Assigning to @ariel-phet for review. Feel free to close if it looks good to you.

ariel-phet commented 10 years ago

The team all agreed this change looked good