Fixed ImageBackground could't be wrapped by Touchable* component #14884

patw0929 · 2017-07-07T08:10:40Z

In 0.46, as warning's advice, I use ImageBackground to replace Images which used as background image wrapper, and in some cases, I also wrap ImageBackground with TouchableHighlight to make it clickable.

But here comes an error:

Touchable child must either be native or forward setNativeProps to a native component

So I pick some code from Image.ios.js into ImageBackground.js to solve it.

Please help to review, thanks!

facebook-github-bot · 2017-07-07T08:10:48Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2017-07-07T08:18:47Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

javache · 2017-07-07T09:18:00Z

React.createClass is deprecated. Can you instead implement setNativeProps on this class and forward it to the child view?

patw0929 · 2017-07-07T09:36:55Z

Got it, I have revised it at 9383abe.
Thank you.

javache · 2017-07-07T09:40:51Z

Libraries/Image/ImageBackground.js

@@ -42,11 +42,23 @@ const View = require('View');
 * ```
 */
 class ImageBackground extends React.Component {
+  setNativeProps(props: Object) {
+    if (this._viewRef) {
+      this._viewRef.setNativeProps(props);


Can you call ensureComponentIsNative here?

Sorry, updated at 0109635.
Thanks again!

facebook-github-bot · 2017-07-07T12:53:30Z

@javache has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

shergin · 2017-07-07T14:48:43Z

Libraries/Image/ImageBackground.js

@@ -42,11 +43,24 @@ const View = require('View');
 * ```
 */
 class ImageBackground extends React.Component {
+  setNativeProps(props: Object) {
+    if (this._viewRef) {
+      ensureComponentIsNative(this._viewRef);


Just curious, why do we need this? We know that is "native".

Ah good point, we're always rendering View. It doesn't hurt to check.

shergin · 2017-07-07T14:49:04Z

Libraries/Image/ImageBackground.js

  render() {
    const {children, style, imageStyle, imageRef, ...props} = this.props;

    return (
-      <View style={style}>
+      <View style={style} ref={this._captureRef}>


N00b question: Will this conflict with ref prop existed in this.props (...)?

Nope, because those go on Image, not View

shergin · 2017-07-07T14:55:59Z

Libraries/Image/ImageBackground.js

+
+  _viewRef: View;
+
+  _captureRef = ref => {


I think we harmlessly can inline it. No?

There's some difference with function binding, which can be expensive. Doing it like this will guarantee it's only bound once I believe.

Summary: This sync includes the following changes: - **[8e25ed20b](facebook/react@8e25ed20b )**: Unify noop and test renderer assertion APIs (#14952) //<Andrew Clark>// - **[870214f37](facebook/react@870214f37 )**: Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps (#14912) //<Eli White>// - **[3989c0950](facebook/react@3989c0950 )**: eslint-plugin-react-hooks@1.3.0 //<Dan Abramov>// - **[1bbfbc98d](facebook/react@1bbfbc98d )**: [ESLint] Add more cases to exhaustive-deps rule (#14930) //<Dan Abramov>// - **[412f88296](facebook/react@412f88296 )**: fix(eslint-plugin-react-hooks): node engine updated to version 7 because of object.entries(#14951) //<Farhad Yasir>// - **[ba708fa79](facebook/react@ba708fa79 )**: Remove ReactNoop.flushDeferredPri and flushUnitsOfWork (#14934) //<Andrew Clark>// - **[920b0bbb3](facebook/react@920b0bbb3 )**: [scheduler] Pass didTimeout argument to callbacks (#14931) //<Andrew Clark>// - **[f99fca3cb](facebook/react@f99fca3cb )**: Fix sample ESLint configuration (#14926) //<Matt Thomson>// - **[22bb94764](facebook/react@22bb94764 )**: Release eslint-plugin-react-hooks@1.2.0 //<Dan Abramov>// - **[a77bbf1a1](facebook/react@a77bbf1a1 )**: [ESLint] Warn against assignments from inside Hooks (#14916) //<Dan Abramov>// - **[219ce8a9c](facebook/react@219ce8a9c )**: Fix tracing fixture (#14917) //<Dan Abramov>// - **[8c1966590](facebook/react@8c1966590 )**: Release 16.8.3 //<Dan Abramov>// - **[7de4d2391](facebook/react@7de4d2391 )**: Fix UMD builds by re-exporting the scheduler priorities (#14914) //<Dan Abramov>// - **[d0318fb3f](facebook/react@d0318fb3f )**: Updating copyright headers, dropping the year (#14893) //<Nathan Hunzaker>// - **[f978d5fde](facebook/react@f978d5fde )**: Fix warning message for new setNativeProps method. on -> with (#14909) //<Eli White>// - **[b0f45c0fc](facebook/react@b0f45c0fc )**: Adding ReactNative.setNativeProps that takes a ref (#14907) //<Eli White>// - **[4f4aa69f1](facebook/react@4f4aa69f1 )**: Adding setNativeProps tests for NativeMethodsMixin (#14901) //<Eli White>// - **[b96b61dc4](facebook/react@b96b61dc4 )**: Use the canonical nativeTag for Fabric's setNativeProps (#14900) //<Eli White>// - **[dab2fdbbb](facebook/react@dab2fdbbb )**: Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies (#14636) //<Dan Abramov>// - **[1493abd7e](facebook/react@1493abd7e )**: Deleted empty App.css (#14149) //<Josh R>// - **[13645d224](facebook/react@13645d224 )**: Deal with fallback content in Partial Hydration (#14884) //<Sebastian Markbåge>// - **[c506ded3b](facebook/react@c506ded3b )**: Don't discard render phase state updates with the eager reducer optimization (#14852) //<Dan Abramov>// - **[0e67969cb](facebook/react@0e67969cb )**: Prompt to include UMD build artifact links in GitHub release (#14864) //<Brian Vaughn>// - **[fad0842fd](facebook/react@fad0842fd )**: Release scripts documentation (#14863) //<Brian Vaughn>// - **[ab7a67b1d](facebook/react@ab7a67b1d )**: Fix react-dom/server context leaks when render stream destroyed early (#14706) //<overlookmotel>// - **[3e5556043](facebook/react@3e5556043 )**: Release 16.8.2 //<Dan Abramov>// - **[dfabb77a9](facebook/react@dfabb77a9 )**: Include another change in 16.8.2 //<Dan Abramov>// - **[c555c008b](facebook/react@c555c008b )**: Include component stack in 'act(...)' warning (#14855) //<Sunil Pai>// - **[ff188d666](facebook/react@ff188d666 )**: Add React 16.8.2 changelog (#14851) //<Dan Abramov>// - **[c4d8ef643](facebook/react@c4d8ef643 )**: Fix typo in code comment (#14836) //<Deniz Susman>// - **[08e955435](facebook/react@08e955435 )**: Statically enable suspense/partial hydration flag in www (#14842) //<Sebastian Markbåge>// - **[0e4135e8c](facebook/react@0e4135e8c )**: Revert "[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802)" (#14839) //<Dan Abramov>// - **[6d4038f0a](facebook/react@6d4038f0a )**: [ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802) //<Rodrigo Ribeiro>// - **[fa6205d52](facebook/react@fa6205d52 )**: Special case crossOrigin for SVG image elements (#14832) //<Brandon Dail>// - **[c6bee765b](facebook/react@c6bee765b )**: Remove false positive warning and add TODOs about `current` being non-null (#14821) //<DanAbramov>// - **[3ae94e188](facebook/react@3ae94e188 )**: Fix ignored sync work in passive effects (#14799) //<Dan Abramov>// - **[f3a14951a](facebook/react@f3a14951a )**: Partial Hydration (#14717) //<Sebastian Markbåge>// Changelog: [GENERAL] [Changed] React sync for revisions f24a0da...22bb947 Reviewed By: gaearon Differential Revision: D14160361 fbshipit-source-id: fffdc922f3ee5dfeeee656a8f213a6d3c03e8481

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 7, 2017

patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from c71d227 to 9383abe Compare July 7, 2017 09:35

patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from 96519ad to 9383abe Compare July 7, 2017 09:37

javache reviewed Jul 7, 2017

View reviewed changes

patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from 9383abe to f94ff87 Compare July 7, 2017 09:48

Fixed ImageBackground could't be wrapped by Touchable* component

0109635

patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from f94ff87 to 0109635 Compare July 7, 2017 10:12

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 7, 2017

shergin reviewed Jul 7, 2017

View reviewed changes

facebook-github-bot closed this in dc97e3f Jul 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed ImageBackground could't be wrapped by Touchable* component #14884

Fixed ImageBackground could't be wrapped by Touchable* component #14884

patw0929 commented Jul 7, 2017

facebook-github-bot commented Jul 7, 2017

facebook-github-bot commented Jul 7, 2017

javache commented Jul 7, 2017

patw0929 commented Jul 7, 2017

javache Jul 7, 2017

patw0929 Jul 7, 2017 •

edited

Loading

facebook-github-bot commented Jul 7, 2017

shergin Jul 7, 2017

javache Jul 7, 2017

shergin Jul 7, 2017

javache Jul 7, 2017

shergin Jul 7, 2017

javache Jul 7, 2017

Fixed ImageBackground could't be wrapped by Touchable* component #14884

Fixed ImageBackground could't be wrapped by Touchable* component #14884

Conversation

patw0929 commented Jul 7, 2017

facebook-github-bot commented Jul 7, 2017

facebook-github-bot commented Jul 7, 2017

javache commented Jul 7, 2017

patw0929 commented Jul 7, 2017

javache Jul 7, 2017

Choose a reason for hiding this comment

patw0929 Jul 7, 2017 • edited Loading

Choose a reason for hiding this comment

facebook-github-bot commented Jul 7, 2017

shergin Jul 7, 2017

Choose a reason for hiding this comment

javache Jul 7, 2017

Choose a reason for hiding this comment

shergin Jul 7, 2017

Choose a reason for hiding this comment

javache Jul 7, 2017

Choose a reason for hiding this comment

shergin Jul 7, 2017

Choose a reason for hiding this comment

javache Jul 7, 2017

Choose a reason for hiding this comment

patw0929 Jul 7, 2017 •

edited

Loading