Added a clickable event to Line, Bar, Pie #155
Conversation
There was a problem hiding this comment.
In addition to all my specific comments, I don't understand what the motivation is/was to making all the Java source changes (targeted specifically at Android) - it appears to be just an alternative UI to start the example app specifically for Android. Is that a fair assessment or is there more intent that isn't apparent to me (current source doesn't compile and I haven't gotten the example app to start successfully yet).
Was your goal to differentiate the example app UI experience between Android and iOS or did you just not want to spend time making them similar?
Why make Java-specific UI changes rather than make javascript source changes and reuse an existing react-native library (like [react-native-android-kit] (https://github.com/adbayb/react-native-android-kit) for example?
In general, what was the reason why you made these Android-specific changes? I'd like to maintain some consistency between the iOS and Android versions in the example app but willing to deviate if it makes sense.
None of them seem necessary to support the goal of adding clickable events when a user clicks on Pie and Bar chart elements. If the Android-specific changes in the example app src would be helpful for others, maybe those specific changes should be submitted in a separate PR.
Also, you add support for a library-user-defined callback functioned to be invoked via the introduction of the chartCallback property in Bar.js, Pie.js, and Line.js, but then I don't see any example Javascript chart source files that provide a value for the chartCallback property demonstrating its use. It would be nice to see a usage example of chartCallback in the example app - maybe just cloning an existing Pie/Bar/Line chart example and implementing chartCallback?
| @@ -15,6 +15,7 @@ coverage.html | |||
|
|
|||
| # Dependency directory | |||
| node_modules | |||
| example/node_modules/ | |||
There was a problem hiding this comment.
This line is unnecessary as the line above it matches files in example/node_modules. Can we remove it?
| @@ -115,7 +115,7 @@ android { | |||
| variant.outputs.each { output -> | |||
| // For each separate APK per architecture, set a unique version code as described here: | |||
| // http://tools.android.com/tech-docs/new-build-system/user-guide/apk-splits | |||
| def versionCodes = ["armeabi-v7a":1, "x86":2] | |||
| def versionCodes = ["armeabi-v7a": 1, "x86": 2] | |||
There was a problem hiding this comment.
Whitespace additions necessary?
| <application | ||
| android:name=".MainApplication" |
There was a problem hiding this comment.
None of this builds cleanly. You get this kind of an error message:
* What went wrong:
Exception while parsing the supplied manifest file .../example/android/app/src/main/AndroidManifest.xml
> The element type "application" must be terminated by the matching end-tag "</application>".```
| public boolean getUseDeveloperSupport() { | ||
| return BuildConfig.DEBUG; | ||
| } | ||
| // private final ReactNativeHost mReactNativeHost = new ReactNativeHost(this) { |
| @@ -29,7 +41,7 @@ public boolean getUseDeveloperSupport() { | |||
| } | |||
| }; | |||
|
|
|||
| @Override | |||
| @Override | |||
There was a problem hiding this comment.
Unnecessary whitespace addition.
| @@ -8,9 +8,10 @@ | |||
| }, | |||
| "dependencies": { | |||
| "moment": "^2.17.1", | |||
| "react": "16.0.0-alpha.6", | |||
There was a problem hiding this comment.
No. We want to use more up-to-date dependencies. Please change these back.
| "react-native-pathjs-charts": "file:../", | ||
| "react": "^15.4.1", | ||
| "react-native": "^0.42.0", | ||
| "react-native-pathjs-charts": "file:///Users/luculent/Documents/Git/react-native-pathjs-charts", |
There was a problem hiding this comment.
Hard-coded user directory won't work for anyone - please change back to file:../
| @@ -37,7 +37,7 @@ class BarChartColumnBasic extends Component { | |||
| title: `Bar (Column) - Basic`, | |||
| }); | |||
| render() { | |||
| let data = [ | |||
| var data = [ | |||
| @@ -99,6 +100,7 @@ class BarChartColumnBasic extends Component { | |||
| showTicks: true, | |||
| zeroAxis: false, | |||
| orient: 'left', | |||
| // max:80, | |||
| @@ -61,7 +62,7 @@ class PieChartBasic extends Component { | |||
| }, | |||
| width: 350, | |||
| height: 350, | |||
| color: '#2980B9', | |||
| color: '#ff0000', | |||
There was a problem hiding this comment.
I am sorry about it. This is unnecessary.
Thank you for contributing a pull request.
Please ensure that you have signed the CLA.