Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KeyboardDragListener violates PhET convention for ES5 set/get, resulting in more differences with DragListener. #1330

Closed
pixelzoom opened this issue Dec 16, 2021 · 3 comments

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Dec 16, 2021

Related to #1307 ...

KeyboardDragListener also deviates from DragListener in how it defines ES5 getters. KeyboardDragListener violates PhET convention and does not have non-ES5 set and get methods.

For example:

// KeyboardDragListener.js
  set transform( transform ) { this._transform = transform; }
  get transform() { return this._transform; }

// DragListener.js
  setTransform( transform ) {...}
  set transform( transform ) { this.setTransform( transform ); }

  getTransform() { 
    return this._transform;
  }
  get transform() { return this.getTransform(); }

This is one more thing to deal with when adding a KeyboardDragListener.

For example, in Geometric Optics, I had this in GORulerNode.js:

   zoomTransformProperty.link( zoomTransform => {
      this.dragListener.setTransform( zoomTransform );
    } );

I want to do the same for a KeyboardDragListener I'm adding to GORulerNode, so I naturally expect to be able to add it like this:

    zoomTransformProperty.link( zoomTransform => {
      this.dragListener.setTransform( zoomTransform );
      keyboardDragListener.setTransform( zoomTransform );
    } );

That fails, because there is no KeyboardDragListener setTransform. So I have to either live with the difference:

      this.dragListener.setTransform( zoomTransform );
      keyboardDragListener.transform = transform;
    } )

... or change working code so that they are the same:

      this.dragListener.transform = zoomTransform;
      keyboardDragListener.transform = transform;
    } )
@zepumph
Copy link
Member

zepumph commented Dec 16, 2021

Sounds about right. I'll fix drag bounds over in #1307.

@zepumph
Copy link
Member

zepumph commented Jan 11, 2022

I updated the API for transform in KeyboardDragListener above. I didn't see any other getters or setters needing work. @pixelzoom is there anything else here?

@pixelzoom
Copy link
Contributor Author

Changes look good, and I see it's being used in geometric-optics. Closing, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants