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

clean up Node's 'on' method #904

Closed
pixelzoom opened this issue Dec 5, 2018 · 4 comments
Closed

clean up Node's 'on' method #904

pixelzoom opened this issue Dec 5, 2018 · 4 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

Requests related to Node's on method:

(1) Document the valid values for the eventName parameter.

(2) Validate the eventName argument value.

(3) Allow subclasses to easily add eventNames.

Related Slack discussion:

Chris Malley [4:26 PM]
Does Node support on( 'visibleBounds',...)? Where do I find the list of valid values for the eventName parameter? Why is there no validation in Node.on? (edited)

Sam Reid [4:27 PM]
Search Node.js for the term ‘.trigger’ (don’t match words)
Node.on extends axon Events which also doesn’t provide validation. We decided Emitter is too heavyweight to use for each Node, but perhaps we should add an Events constructor option that identifies the valid key names.

Chris Malley [4:30 PM]
I never ever would have known to search for “.trigger” to locate valid eventNames. And there’s no failure if I provide an invalid eventName, it just doesn’t work. E.g. if I do on.( 'visible', ... ) instead of on( 'visibility', ...).

Sam Reid [4:31 PM]
If we add Events constructor option “whitelist”, subclasses will need to be able to append to it. For example , Text adds trigger(‘text’)

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Dec 5, 2018

eventName values are apparently in the middle of the doc at the top of the file, at line 159:

 * The following events are exposed non-Scenery usage:
 *
 * - childrenChanged - This is fired only once for any single operation that may change the children of a Node. For
 *                     example, if a Node's children are [ a, b ] and setChildren( [ a, x, y, z ] ) is called on it,
 *                     the childrenChanged event will only be fired once after the entire operation of changing the
 *                     children is completed.
 * - selfBounds - This event can be fired synchronously, and happens with the self-bounds of a Node is changed.
 * - childBounds - This is fired asynchronously (usually as part of a Display.updateDisplay()) when the childBounds of
 *                 the node is changed.
 * - localBounds - This is fired asynchronously (usually as part of a Display.updateDisplay()) when the localBounds of
 *                 the node is changed.
 * - bounds - This is fired asynchronously (usually as part of a Display.updateDisplay()) when the bounds of the node is
 *            changed.
 * - transform - Fired synchronously when the transform (transformation matrix) of a Node is changed. Any change to a
 *               Node's translation/rotation/scale/etc. will trigger this event.
 * - visibility - Fired synchronously when the visibility of the Node is toggled.
 * - opacity - Fired synchronously when the opacity of the Node is changed.
 * - pickability - Fired synchronously when the pickability of the Node is changed
 * - clip - Fired synchronously when the clipArea of the Node is changed.

And based on search of ".trigger", this documentation is stale/incomplete.

Recommended to reference this in @param {string} eventName doc.

@jonathanolson
Copy link
Contributor

It looks like the documentation is up-to-date.

Is it OK to reference the documentation at the top so that it isn't duplicated? Or should I look into moving forward with #490?

If not moving forward with lightweight emitters, should there be a "public" and "scenery-internal" list of events, or should they be combined into one list (like mutator keys)?

@pixelzoom
Copy link
Contributor Author

Is it OK to reference the documentation at the top so that it isn't duplicated? Or should I look into moving forward with #490?

Referencing where @param {string} eventName appears would be fine for now.

If not moving forward with lightweight emitters, should there be a "public" and "scenery-internal" list of events, or should they be combined into one list (like mutator keys)?

Something like that seems necessary for validation. ... if you want to address validation.

@samreid
Copy link
Member

samreid commented Dec 9, 2024

Node.on has been deleted and we use Emitters now. Closing.

@samreid samreid closed this as completed Dec 9, 2024
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

4 participants