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.
http://scenerystack.org/
MIT License
9 stars 6 forks source link

common code for creating screen icons #259

Closed pixelzoom closed 9 years ago

pixelzoom commented 9 years ago

There's a bit of code that I've been copying around for making screen icons. You can see the latest incarnation at HookesLawIconFactory.createScreenIcon. It takes a {Node} icon and puts it on a background rectangle of some specified {Dimension2} size and {Color|string} color.

The code is duplicated in hookes-law and graphing-lines. And I suspect that other developers must be duplicating similar code for screen icons.

My first thought is to generalize as something like joist.ScreenIcon which takes a {Node} icon and {Color|string} fill. Even more general would be a factory function (in scenery-phet?) that puts a node on a background (with options for background size, margins, fill, stroke, etc).

Opinions?

pixelzoom commented 9 years ago

@jbphet's feedback on Skype:

[6/29/15, 9:22:56 AM] John Blanco: I've definitely created code for the former before, so it sounds like a good idea to generalize it. I suppose you could always do both and have the latter reference the former, but I haven't had much cause to put icons on backgrounds other than for screens. [6/29/15, 9:23:40 AM] Chris Malley: thoughts on where such utility functions should live? [6/29/15, 9:32:06 AM] John Blanco: joist.ScreenIcon works for me. For the general function, scenery-phet I suppose. I just glanced through and didn't see any ImageUtil or IconUtil sort of thing. IconWithBackground.js?

pixelzoom commented 9 years ago

I created ScreenIcon, and used it in my sims. I did not create the more general utility function.

Assigning to @jbphet for review.

jbphet commented 9 years ago

For the most part, this looks good. However, I find the names and documentation for xScaleFactor and yScaleFactor a bit confusing. It looks like this:

   xScaleFactor: 0.85, // max percentage of the background width occupied by iconNode, [0,1]
   yScaleFactor: 0.85, // max percentage of the background height occupied by iconNode, [0,1]

The names make me think that the values will be used to scale the icon in the x and y directions, but these are actually used to control the maximum amount of the overall width or height that the icon will occupy. I'd suggest naming these to something like maxIconWidthFactor and maxIconHeightFactor, or maxWidthProportion and maxHeightProportion. Also, I think the correct set notation for the value range is (0,1], since 0 is excluded and 1 is included. Finally, the values are proportions, not percentages.

pixelzoom commented 9 years ago

Options renamed to maxIconWidthProportion and maxIconHeightProportion. Range documentation changed to (0,1].

Assigning to @jbphet for review.

jbphet commented 9 years ago

Looks good - thanks for doing this. Closing.