Skip to content
This repository was archived by the owner on Oct 19, 2018. It is now read-only.

Comments

Attach actual Ruby instance in ref callback#198

Merged
zetachang merged 4 commits intomasterfrom
feature/refs_callback
Dec 26, 2016
Merged

Attach actual Ruby instance in ref callback#198
zetachang merged 4 commits intomasterfrom
feature/refs_callback

Conversation

@zetachang
Copy link
Member

Address #188

@zetachang zetachang merged commit 920ebbb into master Dec 26, 2016
@zetachang zetachang deleted the feature/refs_callback branch December 26, 2016 05:33
Copy link
Contributor

@catmando catmando left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... So I am not actually sure you have to go to all this trouble of distinguishing between dom-node vs .js component vs .rb component.

In the end the "thing" you get back is going to be used (i think) with Element, as in Element[node].focus.

I don't think we can just do a node.focus can we?

If this doesn't make sense perhaps we need a chat?

@catmando
Copy link
Contributor

The other thing I think we should do is make this work like other callbacks as well.

DIV { "hello" }.ref { |node| Element[node].focus }

is the same as

DIV(ref: -> (node) { Element[node].focus }) { "hello" }

options for the name (instead of ref) might be:

DIV { "hello" }.after_mount { ... } or
DIV { "hello" }.on(:mount) { ... } which is pretty sweet...

@zetachang
Copy link
Member Author

Given that the node received is the actual DOM node, so you need to either wrap it inside an opal-jquery element, opal-browser element , node.JS.focus or using backtick.

The reason behind this is that it should be user's decision to decide which kind of encapsulation they want for DOM.

Yeah, providing a callback like method might be helpful.

@catmando
Copy link
Contributor

So bottom line is that today we can't do node.focus (like they do in the JS example) so you have to do either Element[node].focus or node.JS.focus (never used that actually), but my point is that whichever one you use, you should not have to care about what kind of thing (dom node, react.js component or react.rb component) node is. Is that your understanding?

which syntax do you like for the block attacher? on(:mount) seems very consistent...

@zetachang
Copy link
Member Author

zetachang commented Jan 12, 2017 via email

@catmando
Copy link
Contributor

catmando commented Jan 12, 2017

Sure I'm fine with that interpretation of ref, but my point is that we need to make sure it works correctly with Element, and I guess .JS (although again I don't know what that is)

Point being I can't do anything with a node in the ruby environment, since I can't directly say node.focus.

I agree with your point about on(:mount) being inappropriate, however on(:ref) does not read well. How about just .ref { ... }

@zetachang
Copy link
Member Author

zetachang commented Jan 12, 2017 via email

@catmando
Copy link
Contributor

So reading that makes me believe that refnode.JS.focus won't work

@catmando
Copy link
Contributor

My point is that the value sent by ref should act consistently.

@zetachang
Copy link
Member Author

zetachang commented Jan 12, 2017 via email

@zetachang
Copy link
Member Author

Below are the listing of possible ref attached value

  1. hyper-react component, we return the actual Ruby instance
  2. dom-node, we return the actual DOM node in this PR. (which is a non-ruby object)
  3. react-js component, we return the actual reactjs component instance.

I think we could agree 1) is fine to user, for 2) it might be ok if we return a Native wrapped element, but if user want to use another DOM wrapper (opal-browser, or opal-jquery), they will need to unwrap it first (by calling to_n)

(3) is currently not handled carefully, return a Native wrapped element and maybe extend it with React::Component may make it fit the basic use case.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants