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

[WebView] Add props for overriding native component #15016

Closed
wants to merge 10 commits into from

Conversation

cbrevik
Copy link
Contributor

@cbrevik cbrevik commented Jul 14, 2017

Opening a new PR for #10946 (see discussion there).

This PR builds upon #14775 (iOS ViewManager inheritance) and #14261 (more extensible Android WebView).

Motivation
When WebView.android.js and WebView.ios.js use requireNativeComponent, they are hard-coded to require RCTWebView. This means if you want to re-use the same JS-logic, but require a custom native WebView-implementation, you have to duplicate the entire JS-code files.

The same is true if you want to pass through any custom events or props, which you want to set on the custom native WebView.

What I'm trying to solve with this PR is to able to extend native WebView logic, and being able to re-use and extend existing WebView JS-logic.

This is done by adding a new nativeConfig prop on WebView. I've also moved the extra requireNativeComponent config to WebView.extraNativeComponentConfig for easier re-use.

Test plan
@jacobp100 has been kind enough to help me with docs for this new feature. So that is part of the PR and can be read for some information.

I've also created an example app which demonstrates how to use this functionality: https://github.com/cbrevik/webview-native-config-example

If you've implemented the native side as in the example repo above, it should be fairly easy to use from JavaScript like this:

import React, { Component, PropTypes } from 'react';
import { WebView, requireNativeComponent, NativeModules } from 'react-native';
const { CustomWebViewManager } = NativeModules; 

export default class CustomWebView extends Component {
  static propTypes = {
    ...WebView.propTypes,
    finalUrl: PropTypes.string,
    onNavigationCompleted: PropTypes.func,
  };

  _onNavigationCompleted = (event) => {
    const { onNavigationCompleted } = this.props;
    onNavigationCompleted && onNavigationCompleted(event);
  }

  render() {
    return (
      <WebView
        {...this.props}
        nativeConfig={{
          component: RCTCustomWebView,
          props: {
            finalUrl: this.props.finalUrl,
            onNavigationCompleted: this._onNavigationCompleted,
          },
          viewManager: CustomWebViewManager
        }}
      />
    );
  }
}

const RCTCustomWebView = requireNativeComponent(
  'RCTCustomWebView',
  CustomWebView,
  WebView.extraNativeComponentConfig
);

As you see, you require the custom native implementation at the bottom, and send in that along with any custom props with the nativeConfig prop on the WebView. You also send in the viewManager since iOS requires that for startLoadWithResult.

Discussion
As noted in the original PR, this could in principle be done with more React Native components, to make it easier for the community to re-use and extend native components.

@cbrevik cbrevik requested a review from hramos as a code owner July 14, 2017 13:20
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels Jul 14, 2017
@cbrevik
Copy link
Contributor Author

cbrevik commented Aug 4, 2017

@shergin did you have a chance to look at this one yet?

@pull-bot
Copy link

pull-bot commented Aug 24, 2017

Warnings
⚠️

❗ Big PR - This PR is extremely unlikely to get reviewed because it touches 608 lines.

Messages
📖

📄 Docs - Thanks for your contribution to the docs!

@facebook-github-bot label Documentation

@facebook-github-bot large-pr

Attention: @facebook/react-native

Generated by 🚫 dangerJS

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 24, 2017
@facebook-github-bot
Copy link
Contributor

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

@hramos
Copy link
Contributor

hramos commented Aug 30, 2017

It was failing to import due to the "cc:" line in the description, which gets re-parsed by arcanist during import. Edited and trying again.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 30, 2017
@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 31, 2017
@facebook-github-bot
Copy link
Contributor

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

@cbrevik
Copy link
Contributor Author

cbrevik commented Aug 31, 2017

Oops, my bad! I'll remember that for next time.

@facebook-github-bot
Copy link
Contributor

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

@Lxxyx
Copy link
Contributor

Lxxyx commented Sep 11, 2017

I need this too

@cbrevik
Copy link
Contributor Author

cbrevik commented Sep 13, 2017

@hramos Just checking in, and wondering if there's anything holding the merge back?

* @platform ios
*/
viewManager: PropTypes.object,
}),
* Used on Android only, controls whether the given list of URL prefixes should
Copy link
Contributor

@hramos hramos Sep 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's why the PR hasn't landed - missing opening comment. I'll see if I can fix it on our side, no need to update your PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, noted. Seems to be caused by an earlier merge. Thanks! 👍

@dahjelle
Copy link

Does this PR provide a mechanism for customizing WebChromeClient? In particular, I'm trying to get file upload working via onShowFileChooser, but so far it seems that I need to re-implement this code to make that work. Does that seem accurate?

@cbrevik
Copy link
Contributor Author

cbrevik commented Nov 16, 2017

@dahjelle What this PR enables is that you can extend ReactWebViewManager, and override logic as you see fit, while reusing the WebView JavaScript.

So yes, you can override createViewInstance and re-implement it with your own WebChromeClient. See the docs for more info.

(Ideally maybe I should have created a createReactWebViewClient method for even easier overriding of the client. But oh well)

@dahjelle
Copy link

@cbrevik Thanks much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants