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

feature request to add callback for DOMElement.onShow #463

Open
sonwh98 opened this issue Aug 6, 2015 · 12 comments
Open

feature request to add callback for DOMElement.onShow #463

sonwh98 opened this issue Aug 6, 2015 · 12 comments

Comments

@sonwh98
Copy link

sonwh98 commented Aug 6, 2015

I want to mount a react component on a DOMElement when its visible. DOMElement.onShow doesn't take a call back . It would be useful if it did. Can this be a feature request?

@sonwh98
Copy link
Author

sonwh98 commented Aug 6, 2015

There are a bunch of "on" methods in DOMElement.js Isn't it idiomatic for those lifecyle events to take callbacks so that developers can write code to participate in the lifecycle of the component?

@sonwh98 sonwh98 changed the title callback for DOMElement.onShow feature request to add callback for DOMElement.onShow Aug 6, 2015
@sonwh98
Copy link
Author

sonwh98 commented Aug 6, 2015

upon closer inspection it seems these "on" methods are not lifecycle methods. Very confusing because I am use to seeing "on" methods used as lifecycle methods that allow me to put call backs to participate in the lifecycle events. It seems the only way to participate in the lifecycle is through inheritance.

@michaelobriena
Copy link
Member

Yea, sorry for the confusion. The onMethods get called by the nodes when the event occurs. They are not registrations.

@michaelobriena
Copy link
Member

didShow may be a better name

@sonwh98
Copy link
Author

sonwh98 commented Aug 7, 2015

thanks @michaelobriena . is there an easy way to have a call back function triggered when the onShow method is called?

@alexanderGugel
Copy link
Member

@sonwh98 Components enable event handling. The DOMElement's on... methods are used for receiving the events, not for registering handlers. In order to call a function whenever a node is being shown, a new component needs to be added to the Node:

var node = new Node()
node.addComponent({
  onShow: function () {
    // your code goes here
  }
})

@sonwh98
Copy link
Author

sonwh98 commented Aug 8, 2015

@alexanderGuge thanks for answering. These "on" functions are inconsistent. For example, the onMount gets passed in the parent node and the component's id (aka index), but the onShow does not. Is there a reason for this? the onShow should also be passed in the parent node

I'm writing a clojurescript DSL to allow constructing the scene graph declaratively using clojure datastructure. For example:

{:node/id "foobar"
 :node/mount-point [0.5 0.5 0.5] 
 :node/components [ {:onMount (fn [node index] 
                                    (println "onMount node=" node " index=" index]))}
                    {:onShow (fn [] 
                                    (println "onShow called. mount react component"))}]}

It would be nice if the onShow had the same function signature as the onMount so that I can write code to attach a react component onto the famous component's containing Node.

In your example, the node is in the component's lexical scope, but in ClojureScript DSL it is not. The node is constructed later so it needs to be passed in like the onMount

@sonwh98
Copy link
Author

sonwh98 commented Aug 8, 2015

@alexanderGuge I can refactor the onShow and submit a pull request or maintain my own fork (which I prefer not to do). Before I do that, is there a reason for onShow having a different parameter list than onMount?

@alexanderGugel
Copy link
Member

The function signatures are different because the events are different. E.g. onSizeModeChange receives the changed size modes, onTransformChange receives the changed transform, onOpacityChange receives the changed opacity, onAddUIEvent receives the name of the added UI event.

This is not a inconsistency, but a design decision we made. If you decide to ignore the arguments altogether, you can still access them directly via the Node.

We do not intend to change the function signatures, although I agree that it would be "nice" to always pass in the node as a first argument. This would be a breaking change. It is unlikely that it would be merged.

Discussions about changing the way on functions behave is out of scope of this issue IMO.

To address the original issue: The on functions are a way to receive events, they are a replacement for callbacks in this case. Does this answer your original question?

@sonwh98
Copy link
Author

sonwh98 commented Aug 8, 2015

I should have phrased the question differently. I was suggesting that the "on" methods of components be passed the node as the first parameter. Otherwise, if a component needs access to the node in the onShow method, it will have to implement an onMount method and set an internal state variable to the node so that its accessible in the onShow. I'm implementing components from the clojurescript side and without access to the node in the "on" methods, I will have to do what I just describe above and muck around with mutating the "this" pointer. From a Clojure programmer's perspective, this feels dirty.

Even from a Javascript programmer's perspective, having the node passed in as the first parameter of those "on" methods makes things easier. For example, if I needed to do something with the node in the onShow, I would have to implement an onMount that does nothing more than set a node variable on the "this" pointer. This feels like a waste of brain power because someone has to write and implement extra code just because onShow does not have the node passed in. I think you agree that this is good but would require too much work right?

@alexanderGugel
Copy link
Member

This would literally take 5 minutes to change, but it would break every application out there that is using those methods. While I'm a friend of breaking things for the sake of a better API, the proposed change would lead to tons of subtle bugs in dependent code. I'm not sure if we want this.

I agree that the function signature should have been like you proposed from the beginning on.

@michaelobriena Thoughts?

@sonwh98
Copy link
Author

sonwh98 commented Aug 8, 2015

famous hasn't reached 1.0 yet so breaking backwards compatibility for a cleaner API is worth it IMHO. Its a small change not a complete rewrite like what was done a few times already lol... so there is already precedent for breaking backwards compatibility for the better :)

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

No branches or pull requests

3 participants