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

Using evaluateJavascript to fix #9749 #9945

Closed
wants to merge 2 commits into from

Conversation

leeight
Copy link
Contributor

@leeight leeight commented Sep 16, 2016

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request:

Use evaluateJavascript instead of loadUrl if possible, which will preserve the single line comment.

Test plan (required)

Manually.

@ghost
Copy link

ghost commented Sep 16, 2016

By analyzing the blame information on this pull request, we identified @sathyapriya-31 and @jvassbo to be potential reviewers.

@ghost ghost 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 Sep 16, 2016
@leeight
Copy link
Contributor Author

leeight commented Sep 19, 2016

Ping @mkonicek

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2016
@satya164
Copy link
Contributor

Doesn't it mean that the behaviour becomes inconsistent across Android versions? People might test on new Android versions and not realize that the same code will break on older Android versions.

@leeight
Copy link
Contributor Author

leeight commented Sep 19, 2016

@satya164 So it's better tell the differences in the document, and display an YellowBox on older android devices?

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Sep 19, 2016
`evaluateJavascript` will preserve single line comment.
@facebook-github-bot
Copy link
Contributor

@leeight updated the pull request - view changes

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 7, 2016
@leeight
Copy link
Contributor Author

leeight commented Oct 7, 2016

@satya164
image

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 7, 2016
@facebook-github-bot
Copy link
Contributor

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @jacobp100 as a potential reviewer. Could you take a look please or cc someone with more context?

@jacobp100
Copy link
Contributor

LGTM. There will obviously be some edge cases, where the ‘//‘ is in a string, but that probably is not worth trying to resolve.

@@ -207,6 +207,14 @@ class WebView extends React.Component {
console.warn('WebView: `source.body` is not supported when using GET.');
}

if (__DEV__) {
if (this.props.injectedJavaScript
&& this.props.injectedJavaScript.indexOf('//') !== -1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will also cause warning if any string matches this

@leeight
Copy link
Contributor Author

leeight commented Nov 7, 2016

OK, Close it.

@LinusU
Copy link
Contributor

LinusU commented Jul 24, 2018

#20366 should fix this in a way that behaves the same on all versions of Android

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants