Code review for MagnetAccessibleDragHandler.js

[samreid]

From #86

• "for now it doesn't care about holding down keys." Perhaps reword "care about", does it mean it doesn't support held keys and you have to press them repeatedly?
• "so arrow keys act more like a velocity changer than a speed one." What is the difference between velocity and speed in this sentence? Is it correct?
• document the type for direction, it looks like it can be {number|null}
• these properties need JSDoc, visibility annotations and type annotations if public

    this.onDrag = onDrag || function() {};
    this.startDrag = startDrag || function() {};
    this.positionProperty = positionProperty;
    this.model = { direction: DIRECTION_NOT_MOVING, speedIndex: SPEED_INDEX_NOT_MOVING };

• this function calls self in parts and this in parts, it should be made consistent:

    this.keydown = function( event ) {
      this.startDrag();

      if ( Input.isArrowKey( event.keyCode ) ) {
        if ( self.model.direction === event.keyCode ) {
          incrementSpeed();

• the step function should be marked as @public or @private
• line comments need a blank line above them
• These parameters should be described

  /**
   * @param {Property} positionProperty
   * @param {function} startDrag
   * @param {function} onDrag
   * @constructor
   */

I did not realize this file was under active development while I was working on the code review in #86, but it would be good to do a code review on it once it is getting more stable.

[zepumph]

I'm sorry that you even looked at MagnetAccessibleDragHandler. I'm a bit embarrassed, and it was probably just a waste of time. I wrote it quickly prototyping. We should probably discuss further, but in my mind, any code that is only used when a11y is enabled shouldn't be code reviewed because it will be changed in the next few months.

[samreid]

I understand the value of being able to quickly prototype in master--at a minimum, prototype files should be clearly documented as such. But it isn't too much more work to annotate code and document properly as you go (much more difficult to do later on), and this can come in handy if we keep the code for production, or if other developers need to read/understand/maintain the file.

[zepumph]

From here I will keep you more apprised of what is to come for the a11y implementation of FL. Right now the IDRC is working on it, and is fully in the designing phase. They wanted a prototype to play around with. In retrospect, knowing about the stabilization of this sim for phet-io, I should have done this in a branch.

Closing

[samreid]

No worries, the changes look good, thanks!