-
Notifications
You must be signed in to change notification settings - Fork 22
Add setAttribute and trackLocation methods #4
Conversation
437565b
to
c5a3f44
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
README.md
Outdated
```js | ||
import BatchPush from 'react-native-batch-push'; | ||
|
||
// if you want to track one user's location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the user's location?
@@ -1,4 +1,5 @@ | |||
#import "RNBatchPush.h" | |||
#import <React/RCTConvert.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are framework imports, typically they are at the top of the file ;)
@ReactMethod | ||
public void trackLocation(ReadableMap locationMap) { | ||
Location location = new Location("reactNative"); | ||
location.setAccuracy((float) locationMap.getDouble("accuracy")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the docs there are no mentions of accuracy, altitude and speed, would you not rather not set the accuracy, altitude and speed of the Location? So that's the same as iOS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Batch uses Android Location
class. As a result, I am not sure which parameters Batch use. So I decided to construct the best Android Location
instance with all details I have from the returned value of RN.geolocation.getCurrentPosition()
.
For example, in the Batch Dashboard, I see these values:
"geo_accuracy": 17.65,
"geo_last_update": "2018-01-09T17:00:06",
"geo_latitude": 48.8827743,
"geo_longitude": 2.3222816
Would you like to remove these lines anyway ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it doesn't crash if you doesn't provide any accuracy/altitude/speed to the location javascript object, it's fine 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it crashes, maybe update the doc to say that these keys are mandatory on Android
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! 👍
This PR aims to add two new methods: