lucasferreira / react-native-webview-android

Simple React Native Android module to use Android's WebView inside your app
355 stars 158 forks source link

Added support to WebView onMessage() #91

Closed angelstoone closed 6 years ago

angelstoone commented 6 years ago

Hi everyone, I truly need this feature so I have basically continued what was done in the current PR #81 with the right naming that's it.

lucasferreira commented 6 years ago

Hi @angelstoone

Thanks for this good contribution. But, in README sample I think there is a typo:

injectedJavaScript={this.getJavascript()}

I think that by your sample method correct it's:

injectedJavaScript={this.javascriptToInject()}

What do you think?

angelstoone commented 6 years ago

Hi @lucasferreira,

You are right!!! I have just updated it 👍

Let me know if that's ok 😉

BTW thanks for this really good package.

angelstoone commented 6 years ago

Thanks, @lucasferreira. Could you please test if the changes that I have done in the .gitignore file does not break anything given that there isn't a .npmignore file, please? If that's the case I think we should add a .npmignore file to avoid problems 👍

lucasferreira commented 6 years ago

Hi @angelstoone

I had never used .npmignore file before, but you changes in .gitignore seems fine to me.

Thanks for this contribution again ;)

angelstoone commented 6 years ago

The same goes for me @lucasferreira but when I have used npm instead of yarn to install the package, it seemed to me that npm generate the .npmignore file that basically was the .gitignore file. And given that I have added the /node_modules directory in my .gitignore file when using npm to install the package it was not adding the /node_modules of this package causing an error that was not easy to spot. If you need it, you can check this discussion about it on StackOverflow.

lucasferreira commented 6 years ago

Hi @angelstoone,

I don't get you problem, but accord to your suggestion https://stackoverflow.com/a/24942436 only the .gitignore file will do the work if the two files (.npmignore and .gitignore) will have the same content (IMHO that's de case).

angelstoone commented 6 years ago

What I am suggesting is that the .npmignore should be almost the same apart from the node_modules/ entry because we need the other package dependencies to be in the react-native-webview-android package to let be able to work.

Basically, I think that we should add the following .npmignore file:

*.class

# OSX
#
.DS_Store

# node.js
#
# node_modules/ # Basically I think that we should remove this entry for the npm package given that
# we need these modules to be installed.
npm-debug.log
yarn-error.log

# Android/IntelliJ
#
build/
.idea
.gradle
local.properties
*.iml

# Mobile Tools for Java (J2ME)
.mtj.tmp/

# Package Files #
*.jar
*.war
*.ear

# virtual machine crash logs, see http://www.java.com/en/download/help/error_hotspot.xml
hs_err_pid*

Just to let the node_modules to be installed. What do you think? You can easily try it anyway by installing the current package version on master and see if we really need this change to be done.

lucasferreira commented 6 years ago

Can we research in others coolers packages around?

If we look into this module: https://github.com/react-community/react-native-image-picker That's the .gitignore: https://github.com/react-community/react-native-image-picker/blob/develop/.gitignore And that's the .npmignore: https://github.com/react-community/react-native-image-picker/blob/develop/.npmignore

They are different, but both of then had the node_modules ignored.

angelstoone commented 6 years ago

Ok @lucasferreira, we should be fine then 😉