Common Module for FractionNode.js in fractions-intro & fraction-comparison

[michaelm396]

Our current implementation of fractions-intro/FractionNode.js and the established fraction-comparison/FractionNode.js are practically carbon copies. Both programs seem to use very similar properties when attempting to initialize the graphics for the fraction division line and the graphics for the UP/Down spinners.

We would like to generate common code component for FractionNode, however there are differences to the API of FractionNode. In fractions-intro/FractionNode.js s, the constructor is:

/**
   *
   * @param {Property.<number>} numeratorProperty
   * @param {Property.<number>} denominatorProperty
   * @param {Property.<number>} maxNumberOfUnitsProperty
   * @param {Object} [options]
   * @constructor
   */
  function FractionNode( numeratorProperty, denominatorProperty, maxNumberOfUnitsProperty, options )

Whereas, in fraction-comparison/FractionNode.js, the constructor is:

**
   *
   * @param {Property.<number>} numeratorProperty
   * @param {Property.<number>} denominatorProperty
   * @param {Object} [options]
   * @constructor
   */
  function FractionNode( numeratorProperty, denominatorProperty, options ) 

The maxNumberOfUnitsProperty is necessary to determine the numeratorUpEnableProperty and the denominatorDownEnableProperty in fractions-intro/FractionNode.js.

@samreid, I wanted to get your thoughts on implementing a common module which specifically addresses issues regarding the implementation of a graphical fraction display. This implementation would allow for cleaner declarations, a decrease in the amount of duplicate code (especially when multiple fractions have to be declared and displayed), and a more maintainable code base.

[samreid]

Thanks for bringing this up @michaelm396, I think it is a good suggestion and we should factor out FractionNode. Aside from the advantages you pointed out, it will also help ensure a consistent look for fractions across our simulations.

Our codebase currently shows the following declarations of FractionNode:
area-builder/js/game/view/FractionNode.js
fractions-intro/js/intro/view/FractionNode.js
fraction-comparison/js/intro/view/FractionNode.js
trig-tour/js/trig-tour/view/readout/FractionNode.js

The trig-tour one has special handling for square roots in the numerator and we may wish to leave it separate. I agree that it would be good to factor out a single FractionNode for the other cases, and I think it should be added to the "scenery-phet" repo. Can you work on moving FractionNode.js to scenery-phet and updating area builder and fraction comparisons to use it? Also, take a quick look in the trig-tour one to see if there are other features we may generally need (such as negative numbers in fractions).

We should keep the maxNumberOfUnitsProperty parameter, since it is more general. Consider renaming that to maximumProperty since it indicates the maximum value the fraction can take (and number of units is redundant).

Tagging @jbphet since he developed the area-builder one, so he is aware of this probable upcoming change.

I believe @veillette already has commit privileges for scenery-phet, can he be involved with those commits for now? Also, it is important that code going into common code is lint-free and adheres to the PhET code review checklist: https://github.com/phetsims/phet-info/blob/master/checklists/code_review_checklist.md

[jonathanolson]

There was custom behavior needed for fractions-common. @Denz1994 do you think MixedFractionNode.js would be suitable for moving to common code?

[Denz1994]

I will be able to give a better answer once reviewed. Tagging phetsims/fractions-common#29 to consider for common code.

[Denz1994]

It seems like MixedFractionNode.js would be useful as a common code component. The constructor only requires a options object and handles default values well.

I would suggest the adding an options.vinculumNodeWidth: 10 // {number} so the declation looks like this.vinculumNode = new Line(0,0,options.viculumNodeWidth,0).

Font sizes are checked with an assertion in BuildingScreenView.js. I'm noting this because it is a nice feature, but I don't know how applicable it would be in common code.

assert && assert( ICON_DESIGN_BOUNDS.containsBounds( label.bounds ), 'Sanity check for level icon layout' );
assert && assert( ICON_DESIGN_BOUNDS.containsBounds( iconContainer.bounds ), 'Sanity check for level icon layout' );

[jonathanolson]

We have vinculumExtension which I believe is better than explicitly declaring the vinculum width.

@ariel-phet, this issue would mainly be moving fractions-common's fractions display code to common code, and updating trig-tour/area-builder/fraction-comparison. How should I prioritize this?

[ariel-phet]

@jonathanolson I think you could move it to common code, and merely make issues in the other repos to use it at some point in the future, those sims do not need to be updated right now.

[jonathanolson]

@ariel-phet I've moved MixedFractionNode and PropertyFractionNode to common code. Should we create issues in other sims to use them? I'm not sure exactly where would be appropriate.

[ariel-phet]

@jonathanolson I don't think we necessarily need other sims to use them.

@amanda-phet should be aware that these nodes now exist in common code (and are used by the fractions suite). I don't think there are any sims that need to be retrofitted.

@amanda-phet if you agree (no sims need to be retrofitted), feel free to close and just keep this in mind if designing a sim with fraction representations to alert the dev.

[amanda-phet]

Thanks for letting me know. I created issues in relevant sim repos (that I could think of, since many of these are probably using custom layouts).