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

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

Conversation

@patw0929
Copy link
Contributor

@patw0929 patw0929 commented Jul 7, 2017

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

2017-07-07 3 25 19

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

Please help to review, thanks!

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jul 7, 2017

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
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jul 7, 2017

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

@javache
Copy link
Member

@javache javache commented Jul 7, 2017

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

@patw0929 patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from c71d227 to 9383abe Jul 7, 2017
@patw0929
Copy link
Contributor Author

@patw0929 patw0929 commented Jul 7, 2017

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

@patw0929 patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from 96519ad to 9383abe Jul 7, 2017
@@ -42,11 +42,23 @@ const View = require('View');
* ```
*/
class ImageBackground extends React.Component {
setNativeProps(props: Object) {
if (this._viewRef) {
this._viewRef.setNativeProps(props);
Copy link
Member

@javache javache Jul 7, 2017

Can you call ensureComponentIsNative here?

Copy link
Contributor Author

@patw0929 patw0929 Jul 7, 2017

Sorry, updated at 0109635.
Thanks again!

@patw0929 patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from 9383abe to f94ff87 Jul 7, 2017
@patw0929 patw0929 force-pushed the feature/fix-imagebackground-wrapped-by-touchable-component branch from f94ff87 to 0109635 Jul 7, 2017
@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Jul 7, 2017

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

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

@shergin shergin Jul 7, 2017

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

Copy link
Member

@javache javache Jul 7, 2017

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

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

return (
<View style={style}>
<View style={style} ref={this._captureRef}>
Copy link
Contributor

@shergin shergin Jul 7, 2017

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

Copy link
Member

@javache javache Jul 7, 2017

Nope, because those go on Image, not View


_viewRef: View;

_captureRef = ref => {
Copy link
Contributor

@shergin shergin Jul 7, 2017

I think we harmlessly can inline it. No?

Copy link
Member

@javache javache Jul 7, 2017

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

facebook-github-bot added a commit that referenced this issue Feb 27, 2019
Summary:
This sync includes the following changes:

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

- **[8e25ed20b](facebook/react@8e25ed2 )**: Unify noop and test renderer assertion APIs (#14952) //<Andrew Clark>//
- **[870214f37](facebook/react@870214f )**: Deprecate ref.setNativeProps in favor of ReactNative.setNativeProps (#14912) //<Eli White>//
- **[3989c0950](facebook/react@3989c09 )**: eslint-plugin-react-hooks@1.3.0 //<Dan Abramov>//
- **[1bbfbc98d](facebook/react@1bbfbc9 )**: [ESLint] Add more cases to exhaustive-deps rule (#14930) //<Dan Abramov>//
- **[412f88296](facebook/react@412f882 )**: fix(eslint-plugin-react-hooks): node engine updated to version 7 because of object.entries(#14951) //<Farhad Yasir>//
- **[ba708fa79](facebook/react@ba708fa )**: Remove ReactNoop.flushDeferredPri and flushUnitsOfWork (#14934) //<Andrew Clark>//
- **[920b0bbb3](facebook/react@920b0bb )**: [scheduler] Pass didTimeout argument to callbacks (#14931) //<Andrew Clark>//
- **[f99fca3cb](facebook/react@f99fca3 )**: Fix sample ESLint configuration (#14926) //<Matt Thomson>//
- **[22bb94764](facebook/react@22bb947 )**: Release eslint-plugin-react-hooks@1.2.0 //<Dan Abramov>//
- **[a77bbf1a1](facebook/react@a77bbf1 )**: [ESLint] Warn against assignments from inside Hooks (#14916) //<Dan Abramov>//
- **[219ce8a9c](facebook/react@219ce8a )**: Fix tracing fixture (#14917) //<Dan Abramov>//
- **[8c1966590](facebook/react@8c19665 )**: Release 16.8.3 //<Dan Abramov>//
- **[7de4d2391](facebook/react@7de4d23 )**: Fix UMD builds by re-exporting the scheduler priorities (#14914) //<Dan Abramov>//
- **[d0318fb3f](facebook/react@d0318fb )**: Updating copyright headers, dropping the year (#14893) //<Nathan Hunzaker>//
- **[f978d5fde](facebook/react@f978d5f )**: Fix warning message for new setNativeProps method. on -> with (#14909) //<Eli White>//
- **[b0f45c0fc](facebook/react@b0f45c0 )**: Adding ReactNative.setNativeProps that takes a ref (#14907) //<Eli White>//
- **[4f4aa69f1](facebook/react@4f4aa69 )**: Adding setNativeProps tests for NativeMethodsMixin (#14901) //<Eli White>//
- **[b96b61dc4](facebook/react@b96b61d )**: Use the canonical nativeTag for Fabric's setNativeProps (#14900) //<Eli White>//
- **[dab2fdbbb](facebook/react@dab2fdb )**: Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies (#14636) //<Dan Abramov>//
- **[1493abd7e](facebook/react@1493abd )**: Deleted empty App.css (#14149) //<Josh R>//
- **[13645d224](facebook/react@13645d2 )**: Deal with fallback content in Partial Hydration (#14884) //<Sebastian Markbåge>//
- **[c506ded3b](facebook/react@c506ded )**: Don't discard render phase state updates with the eager reducer optimization (#14852) //<Dan Abramov>//
- **[0e67969cb](facebook/react@0e67969 )**: Prompt to include UMD build artifact links in GitHub release (#14864) //<Brian Vaughn>//
- **[fad0842fd](facebook/react@fad0842 )**: Release scripts documentation (#14863) //<Brian Vaughn>//
- **[ab7a67b1d](facebook/react@ab7a67b )**: Fix react-dom/server context leaks when render stream destroyed early (#14706) //<overlookmotel>//
- **[3e5556043](facebook/react@3e55560 )**: Release 16.8.2 //<Dan Abramov>//
- **[dfabb77a9](facebook/react@dfabb77 )**: Include another change in 16.8.2 //<Dan Abramov>//
- **[c555c008b](facebook/react@c555c00 )**: Include component stack in 'act(...)' warning (#14855) //<Sunil Pai>//
- **[ff188d666](facebook/react@ff188d6 )**: Add React 16.8.2 changelog (#14851) //<Dan Abramov>//
- **[c4d8ef643](facebook/react@c4d8ef6 )**: Fix typo in code comment (#14836) //<Deniz Susman>//
- **[08e955435](facebook/react@08e9554 )**: Statically enable suspense/partial hydration flag in www (#14842) //<Sebastian Markbåge>//
- **[0e4135e8c](facebook/react@0e4135e )**: Revert "[ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802)" (#14839) //<Dan Abramov>//
- **[6d4038f0a](facebook/react@6d4038f )**: [ShallowRenderer] Queue/rerender on dispatched action after render component with hooks (#14802) //<Rodrigo Ribeiro>//
- **[fa6205d52](facebook/react@fa6205d )**: Special case crossOrigin for SVG image elements (#14832) //<Brandon Dail>//
- **[c6bee765b](facebook/react@c6bee76 )**: Remove false positive warning and add TODOs about `current` being non-null (#14821) //<DanAbramov>//
- **[3ae94e188](facebook/react@3ae94e1 )**: Fix ignored sync work in passive effects (#14799) //<Dan Abramov>//
- **[f3a14951a](facebook/react@f3a1495 )**: 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants