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

Referencing issue #172

Closed
madisvain opened this issue Oct 10, 2016 · 17 comments
Closed

Referencing issue #172

madisvain opened this issue Oct 10, 2016 · 17 comments

Comments

@madisvain
Copy link

Using refs for components. I have not tested other components but it seems using refs on for example Input component does not work and the input is not made available. If I replace this with a regular input then everything works as expected.

https://facebook.github.io/react/docs/more-about-refs.html

I used the input like this:

<Input
  ref={ (ref) => this._input = ref }
  { ...input }
  type="text"
  onChange={ this.handleInputChange }
  onFocus={ this.handleInputFocus }
  onBlur={ this.handleInputBlur }
/>

With the datepicker overylay example from here:
http://react-day-picker.js.org/examples/?overlay

@madisvain
Copy link
Author

Not to be confused with the render() function that you define on your component (and which returns a virtual DOM element), ReactDOM.render() will return a reference to your component's backing instance (or null for stateless components).

So they will not work for stateless components. Ref's are not usually needed but the use case especially for inputs is surely there.

@madisvain
Copy link
Author

Actually one more note:
facebook/react#4936 (comment)

It would be quite simple to add support for them to stateless components also without making them have state.

@TheSharpieOne
Copy link
Member

I'm guessing that you want the ref in order to access the value of the input when the onChange, onFocus, and onBlur events are called?

If that is the case, save the trouble and don't use ref. The event, e in the timepicker example you linked, contains a ref to the input, event.target (e.target, in the example)
The whole this.input.blur() and this.input.focus() can be more implicit by using e.target.blur() and e.target.focus() in that example. Plus, doing it this way means if you have multiple Inputs, you don't have to manage multiple refs.
Personally, I built a whole validation library around the inputs reactstrap exposes and did not need any refs. I was also able to hook up a different datepicker (https://github.com/YouCanBookMe/react-datetime) (though that datepicker has it's own issues).

As you mentioned, ref's are not usually needed... But there probably should be a way to get a ref (for other reason)... Most of the components are stateless, which poses an issue. The comment in the react repo with an example is mostly showing how to get a ref for children DOM, plus the whole "getRef" style prop seems dirty. @eddywashere would have to chime in if there should be a way to access refs and, if so, which way (via "prop-in-the-middle" or make components not stateless).

@eddywashere
Copy link
Member

Good discussion! Thanks @TheSharpieOne for your feedback. I was going to ping you here to see what you thought :)

I've been less inclined to hook into a ref for focus just over personal preference. Besides Input, do other components need ref exposed for them? I think an input is the unique example that breaks down the stateless functional component. The react docs for "refs to component" use the ref callback to set focus on an input. There's probably a few more use cases for it. That alone is worth switching this over to a normal component instead of the prop in the middle.

The prop in the middle is a weird thing to document and encourage. I'm hesitant to switch the other functional components over. If there's a reasonable need, like input has, feel free to discuss them here or open a separate issue to track the discussion/request.

Does that work for you @madisvain?

@eddywashere
Copy link
Member

Released 3.4.1 to enable refs on Input. Closing this issue out. @madisvain if there are other components that should be switched over, please create a separate issue for them with a use case. Thanks!

@CrisLi
Copy link

CrisLi commented Oct 28, 2016

@eddywashere

Can you provide a demo code to the doc? (reactstrap v3.7.0)

I am using the ref in the input component, however, I only can get the react element, but not the input DOM. Would you expose the input DOM?

            <Input
              type="text"
              name="username"
              ref={(input) => (this.username = input)}
              id="username"
              defaultValue='root' />

console.log(this.username);

Input {props: Object, context: Object, refs: Object, updater: Object, _reactInternalInstance: ReactCompositeComponentWrapper…}

@eddywashere
Copy link
Member

@CrisLi thanks for pinging me. Reopening this issue. I tried putting together a demo and came across this warning: https://facebook.github.io/react/warnings/special-props.html

Looks like a prop like getRef to pass through ref is the way to go.

@eddywashere
Copy link
Member

transferring from @ajainarayanan in #218 (comment)

@TheSharpieOne I agree that refs could be used to set focus on an input element but wouldn't it be more straight forward by accepting a focus(or autoFocus) attribute for focus alone? We could also have a getRef attribute that will return the underlying input element's ref and can be used independently.

Its more of getting the element ref and focusing it vs setting the attribute of element and make itself focus.

@TheSharpieOne
Copy link
Member

@ajainarayanan Yeah, I couldn't find a good reason to need a ref without doing something that could be done in a more explicit react way. Focus was the only thing I could think of (and is the only example the react docs has for them).
Only certain elements are focusable (unless you start adding tabindex to random things) The PR passes the ref via getRef just the same, you get the underlying element's ref (such as the input, anchor, or button). I didn't add the attribute to everything, some things don't really make sense to have a ref of (such as things with multiple elements generated). I'd say feel free to create another PR adding the ability to other components if you believe they will benefit from it.
To me, using refs is a last resort because 99% of the time there is a better, more reliable way to do what you want without using ref.

@eddywashere
Copy link
Member

^ +1. Closing this out as getRef was added in https://github.com/reactstrap/reactstrap/pull/218/files

example:

<Input
  type="text"
  name="username"
  getRef={(input) => (this.username = input)}
  id="username"
  defaultValue='root' />

@placidusax-in-storm
Copy link

ok , the getRef is work fine,thinks

@bleurubin
Copy link

for anyone that comes across this, looks like the prop has been changed from getRef to innerRef:

https://github.com/reactstrap/reactstrap/blob/5.0.0-alpha.3/src/Input.js#L15

@Hum4n01d
Copy link

How would I access methods like onChange?

@TheSharpieOne
Copy link
Member

@Hum4n01d by providing the onChange prop.
Every prop the component receives is passed down, but ref is special as react itself consumes it and doesn't pass it down. That is why there is a special case for ref.

@Hum4n01d
Copy link

Hum4n01d commented Nov 10, 2017 via email

@Kielan
Copy link

Kielan commented Dec 29, 2017

this is very unclear to other devs using reactstrap, I was lucky to find this by google or I would never have known, unless I started reading through the source code (not clear this dependency is breaking refs btw) also refs are required with some third party libraries so it should be of some importance to not break a core feature of the framework you build a library for. innerRef should be changes back to ref and properly go to the input component.

Please let me know why there would be any objection to this.

@TheSharpieOne
Copy link
Member

@Kielan reactstrap doesn't have that ability, that is core react. ReactJS defines ref and how it is used. React does NOT pass ref down, it consumes it. ref is a react thing, not reactstrap. You can use ref all you want, but it will not do what you think it would do. Since many of the reactstrap components are functional, ref doesn't do anything (https://reactjs.org/docs/refs-and-the-dom.html#refs-and-functional-components). For the ones (like Input) which are not functional components, you would get a ref to the component (which you probably didn't want) and not the DOM input (which you probably wanted)

While you could add a ref to the child component, this is not an ideal solution, as you would only get a component instance rather than a DOM node. Additionally, this wouldn’t work with functional components.

Ref: https://reactjs.org/docs/refs-and-the-dom.html#exposing-dom-refs-to-parent-components

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

8 participants