Review recent layout changes

[zepumph]

I just did a fair amount of layout change, and would love some feedback. I want to make sure that it uses practices that good for PhET i18n and have good coding sense. @ariel-phet could you please assign to someone to look over the general layout (it's not too big of a sim), focusing around commits made in #62.

I want to be sure that I'm not using too many fixed values (empirically determined).

[ariel-phet]

@jessegreenberg do you have time for this review?

If not please assign back to me.

[jessegreenberg]

Sure, Ill take a look!

[jessegreenberg]

I can look at this on 7/19/17 or 7/20/17.

[phet-steele]

This is for phetsims/QA/issues/24, sorta.

[jessegreenberg]

Talked with @zepumph, it sounds like I should take a look at commits against #62, but there may be other commits so I should look at others in master

[jessegreenberg]

Gonna start here 91d3f2ef28a9f4a3f20c19b261211691b1e65d24 and move up!

[jessegreenberg]

Changes look great @zepumph, these are some big improvements. Changes for the layout are excellent. I have just a few picky comments about some code style and structure.

• In https://github.com/phetsims/ohms-law/commit/4275a831f9661101acaa8338c7eb67563c448987, getMaxCurrent was added as a public function to OhmsLawModel. With no reference to this, getMaxCurrentcan be a static function. This is useful because you don't need to have an instance of an OhmsLawModel to use the function. You no longer need to modify the constructor signature of ReadoutPanel to take the model, ReadoutPanel can still take currentProperty, and the currentValue text can use OhmsLawModel.getMaxCurrent().

• In OhmsLawModel, computeCurrent was added to the top of the file replacing an anonymous function. Typically, these are placed below the constructor so the file layout looks like

  // modules
  // constants
  function MyType() {}
  mySim.register( 'MyType', MyType );
  inherit( SuperType, MyType );
  return MyType;

  Placing the constructor at the top is just a little easier to read/find (especially if there are lots of these functions).

• Can you please clarify the comment on line 89 of SliderUnit?

• In https://github.com/phetsims/ohms-law/commit/fe0e7c4fa87a748a4cd175f7960189448a10fe2b, BATTERY_WIDTH was removed from constants and calculated in BatteriesView constructor. But those values are still constants, and are derived from constants, so they could be added to OhmsLawConstants. This might look something like

// constants
var VOLTAGE_RANGE = new Range();
var AA_VOLTAGE = new Range();

var OhmsLawConstants = {
  VOLTAGE_RANGE: VOLTAGE_RANGE,
  AA_VOLTAGE: AA_VOLTAGE,
  MAX_NUM_BATTERIES: Math.ceil( VOLTAGE_RANGE.max / AA_VOLTAGE )
  ...
};

Again, the benefit is that BatteryView doesn't need a batteryWidth argument, it can just pull from the constants file.

Nice work @zepumph, let me know if you have any questions about the above comments.

[zepumph]

Can you please clarify the comment on line 89 of SliderUnit?

I updated the comment to // Size the unit to be as big as possible next to the value with spacing.

Does this make sense?

All other changes fixed. Thanks for the review!

[zepumph]

Unassigning, @jessegreenberg let me know if you need anything else.

[jessegreenberg]

Thanks for the ping @zepumph, sorry I let this lag for a while. Changes look great. Thanks!