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

improve Android code integration documentation #152

Merged
merged 4 commits into from Jan 30, 2018

Conversation

slorber
Copy link
Contributor

@slorber slorber commented Nov 23, 2017

No description provided.

@slorber
Copy link
Contributor Author

slorber commented Nov 23, 2017

can be closed after merge: #111

@henrikra
Copy link
Contributor

Great improvements 👍

@henrikra
Copy link
Contributor

I would put xmlns:tools="http://schemas.android.com/tools" inside of example code of AndroidManifest so you won't miss it easily :)

README.md Outdated
import com.robinpowered.react.Intercom.IntercomPackage;
import io.intercom.android.sdk.Intercom;

public class MainApplication extends MultiDexApplication {
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this MultiDexApplication?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm actually I don't know, it's what you get when you eject from Expo (I think multidex is used for large Android applications)

@slorber
Copy link
Contributor Author

slorber commented Nov 23, 2017

@henrikra thanks for the reveiw I'e just made some changes

README.md Outdated
<?xml version="1.0" encoding="utf-8"?>
<manifest package="com.myapp"
...
xmlns:tools="http://schemas.android.com/tools"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add comment like <!-- Add this line -->

Copy link
Contributor Author

@slorber slorber Nov 23, 2017

Choose a reason for hiding this comment

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

it wouldn't be valid xml comment imo so I prefered to put "..."

You want something like that? as you can see it breaks syntax highlighting on github so I prefered ...

<manifest package="com.myapp"
          <!-- add this line -->
          xmlns:tools="http://schemas.android.com/tools"
    >

   <!-- comment -->
</manifest>

Copy link
Contributor

Choose a reason for hiding this comment

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

What ever works the best :D Point is that there is comment pointing what you need to add

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this could be fine like this:

image

@mnogueron
Copy link

Really useful PR, hope that it will be merge soon for others! :)

@browniefed browniefed merged commit 0a8efd4 into tinycreative:master Jan 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants