Skip to content

Fix passing ref to functional components (#153) #156

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

Closed
wants to merge 2 commits into from

Conversation

mogzol
Copy link
Contributor

@mogzol mogzol commented Dec 10, 2019

See issue #153

Previously, using a functional component as the child of a Trigger would cause React to give console errors due to rc-trigger passing a ref to the child, which is not allowed in functional components. This adds a check for functional components and avoids passing them a ref when encountered. Other components (classes and plain elements) are not affected, and will behave the same as before.

@vercel
Copy link

vercel bot commented Dec 10, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/react-component/trigger/1y8hdnime
✅ Preview: https://trigger-git-fork-mogzol-master.react-component.now.sh

@codecov
Copy link

codecov bot commented Dec 10, 2019

Codecov Report

Merging #156 into master will increase coverage by 0.14%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #156      +/-   ##
==========================================
+ Coverage   85.27%   85.42%   +0.14%     
==========================================
  Files           7        7              
  Lines         489      494       +5     
  Branches      136      140       +4     
==========================================
+ Hits          417      422       +5     
  Misses         72       72
Impacted Files Coverage Δ
src/index.tsx 82.22% <100%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cd87ca...524166e. Read the comment docs.

@mogzol
Copy link
Contributor Author

mogzol commented Jan 10, 2020

Updated to resolve conflicts. @zombieJ any chance of getting this (and also #158) merged? They are affecting my ability to use rc-trigger in my projects.

@zombieJ
Copy link
Member

zombieJ commented Jan 13, 2020

hi @mogzolm, this warning is by expect. Since we don't want user missing passing ref and have to use findDOMNode which is mark as deprecated in StrictMode.

ref:

return ReactDOM.findDOMNode(this) as HTMLElement;

@mogzol
Copy link
Contributor Author

mogzol commented Jan 13, 2020

Ah, I see. OK so rc-trigger can't really support function components then (without using findDOMNode). So the proper solution is for users to modify their code so that any child of rc-trigger will accept refs (with something like forwardRef, or rewriting the components into class components).

Alright, that's fair, I'll close this.

@mogzol mogzol closed this Jan 13, 2020
@zombieJ
Copy link
Member

zombieJ commented Jan 14, 2020

Yes. Before fragment refs RFC merge, we still need this as fallback.

ref: reactjs/rfcs#97

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

Successfully merging this pull request may close these issues.

2 participants