[ListView] Use function refs and support composed refs #6441
Conversation
Fixes an issue where if you implement `renderScrollComponent` and have a `ref` callback on the returned element, the ref is not clobbered by the ref that ListView adds to the the element. This is accomplished by converting the ref from a legacy string-based ref to a callback-based ref, and then using `cloneReferencedElement`, which is a simple utility to compose callback refs. Test Plan: Open the ListView examples in UIExplorer
|
Note that this adds a small dep to package.json, so it'll be necessary to rebuild npm-shrinkwrap.json. |
|
@ide updated the pull request. |
|
@ide updated the pull request. |
|
FYI @ide we removed shrinkwrap from master (but are still shipping it to NPM), so no need to worry about dep updates and updating shrinkwrap for PRs anymore. |
|
Looks good to me. |
|
@facebook-github-bot shipit |
facebook-github-bot
added
GH Review: accepted
and removed
GH Review: review-needed
labels
|
Thanks for importing. If you are an FB employee go to Phabricator to review. |
|
@mkonicek this failed for some reason...not sure why |
|
The error is: It's likely because this PR changes package.json and need to import it manually into the fb npm cache. Any PRs that touch package.json need manual import. Can you do this without adding a dependency? Just an idea. |
|
I'll let @ide speak to that. It's a small dep...just a function...it could possibly live within RN...but shouldn't be a big deal to add as a dependency. |
|
It's a small dep and pretty stable unless React's ref API changes. There is at least one other place in the codebase (Navigator) that needs this behavior too, though, so having it be a shared helper function makes the most sense to me. And most React libs that clone elements and set refs should be using this, so that's why the helper is published to npm. |
|
OK that makes sense, need to find some time to import |
|
catping =)
|
|
Haha nice macro! :) Sorry for the delay, I'll try to merge this tomorrow. |
|
Removing the 'Import failed' label on this one. Using the 'Import failed' to track random failures we can fix. Automatically merging PRs that change package.json is unsupported and likely won't be for quite a long time (although it should be possible to implement). |
|
Trying to merge it internally now, sorry for the (long) delay! |
|
It's failing with an error installing node_modules from the internal cache in our CI. Looking into it, might have to wait for people in California. Merging PRs that change package.json is hard unfortunately. |
ghost
closed this
in
|
It worked! 🎉 Sorry I forgot about it for 13 days :( Feel free to ping me on messenger next time. |
|
@mkonicek thanks for pushing it though - appreciate it =) |
|
I'd recommend wrapping instead. Makes it a bit more seamless, doesn't take on another dependency, and is backwards compatible with old-style refs. |
Summary:Fixes an issue where if you implement `renderScrollComponent` and have a `ref` callback on the returned element, the ref used to be clobbered by the ref that ListView adds to the element. This is accomplished by converting the ref from a legacy string-based ref to a callback-based ref, and then using `cloneReferencedElement`, which is a simple utility to compose callback refs. Closes facebook#6441 Differential Revision: D3064250 Pulled By: mkonicek fb-gh-sync-id: 2d55d04e2144a1cc08900a57a1fc0dab07c87eea fbshipit-source-id: 2d55d04e2144a1cc08900a57a1fc0dab07c87eea
Fixes an issue where if you implement
renderScrollComponentand have arefcallback on the returned element, the ref used to be clobbered by the ref that ListView adds to the element.This is accomplished by converting the ref from a legacy string-based ref to a callback-based ref, and then using
cloneReferencedElement, which is a simple utility to compose callback refs.Test Plan: Open the ListView examples in UIExplorer (iOS)