-
Notifications
You must be signed in to change notification settings - Fork 249
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
Comments
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? |
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. |
Yea, sorry for the confusion. The onMethods get called by the nodes when the event occurs. They are not registrations. |
didShow may be a better name |
thanks @michaelobriena . is there an easy way to have a call back function triggered when the onShow method is called? |
@sonwh98 Components enable event handling. The DOMElement's var node = new Node()
node.addComponent({
onShow: function () {
// your code goes here
}
}) |
@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:
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 |
@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? |
The function signatures are different because the events are different. E.g. 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 To address the original issue: The |
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? |
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? |
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 :) |
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?
The text was updated successfully, but these errors were encountered: