phetsims / expression-exchange

"Expression Exchange" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

creator searches for parent ScreenView #81

Closed pixelzoom closed 7 years ago

pixelzoom commented 7 years ago

I stumbled upon this while looking at https://github.com/phetsims/projectile-motion/issues/20.

Regarding this pattern in CoinTermCreatorNode, line 133:

        // find the parent screen if not already found by moving up the scene graph
        if ( !parentScreenView ) {
          var testNode = self;
          while ( testNode !== null ) {
            if ( testNode instanceof ScreenView ) {
              parentScreenView = testNode;
              break;
            }
            testNode = testNode.parents[ 0 ]; // move up the scene graph by one level
          }
          assert && assert( parentScreenView, 'unable to find parent screen view' );
        }

In https://github.com/phetsims/scenery-phet/issues/214#issuecomment-174014179 and in subsequent developer-meeting discussions, this approach was identified as undesirable. And we discussed better ways to do this. That was 1/22/2016. Why is this pattern still be using in this brand new sim?

jbphet commented 7 years ago

I think this is essentially a duplicate of https://github.com/phetsims/expression-exchange/issues/58, but I'll leave both open until the issue is addressed.

jbphet commented 7 years ago

I've implemented event forwarding for this, and it appears to be working well. There is one TODO for documentation remaining, but I'll track that in #58 and will make sure it gets addressed. @pixelzoom - do you want to review since you logged this issue? You're welcome to pass and wait until the code review if you prefer.

jbphet commented 7 years ago

Hang on - based on some examples that @samreid pointed me to, I think this can be simplified, so I'm assigning it back to me.

pixelzoom commented 7 years ago

@jbphet wrote:

I've implemented event forwarding for this, and it appears to be working well. ...

(1) @samreid recommended using the approach described in https://github.com/phetsims/scenery-phet/issues/214#issuecomment-296881631, and I thought we were all on the same page for that. What you've implemented doesn't seem to correspond to that.

(2) You are now calling private functions in MovableDragHandler (movableDragHandlerStart, movableDragHandlerEnd).

pixelzoom commented 7 years ago

Looks great, and matches the approach in phetsims/scenery-phet#214 (comment).

Closing.