Add a map for virtual beacons by gmarty · Pull Request #340 · mozilla-magnet/magnet-client

gmarty · 2016-11-17T16:11:47Z

No description provided.

wilsonpage · 2016-11-18T14:46:06Z

android/app/build.gradle

 dependencies {
    compile project(':react-native-linear-gradient')
    compile project(':magnet-scanner-android')
+    compile project(':reactnativemapboxgl')


Any reason why we don't use default react-native MapView?

The default map component is iOS only.

Also I tried the react-native-maps by Airbnb that the React Native team recommends, but it ended up being a massive mess to set up and configure and it isn't even compatible with our Google Play Service version. So falling back to mapbox was the best solution.

Interesting, do you have any more info on the google-play-services issue?

wilsonpage · 2016-11-18T14:46:32Z

android/app/src/main/AndroidManifest.xml

            android:name=".notifications.NotificationService"
            android:enabled="true"
            android:exported="false"/>
+        <service android:name="com.mapbox.mapboxsdk.telemetry.TelemetryService" />


What's this for?

That's for mapbox telemetry. This is part of the settings process. I'm not sure if deactivating it can have side effects.
From a purely user standpoint the telemetry can be deactivated through the map interface.
Do you want me to enquire it?

Please do, I'm afraid of what legal reasons we will need to add in our side in order to use this.

wilsonpage · 2016-11-18T14:49:31Z

lib/store/actions.js

+  return {
+    type: 'LOCATION_CHANGED',
+    pos,
+  };


Changes look spot on, nice one!

wilsonpage · 2016-11-18T14:50:50Z

lib/store/reducer.js


-module.exports = (state, action) => {
-  if (!state) return initialState;
+module.exports = (state = initialState, action) => {


wilsonpage · 2016-11-18T14:51:51Z

lib/store/reducer.js

    case 'UNSUBSCRIBED': return unsubscribed(state, action);
    case 'SUBSCRIPTION_UPDATED': return subscriptionUpdated(state, action);
+    case 'LOCATION_UPDATE_STARTED': return startLocationUpdates(state);
+    case 'LOCATION_UPDATE_STOPPED': return stopLocationUpdates(state);


NIT: UPDATES

wilsonpage · 2016-11-18T14:53:04Z

lib/views/item-scene.js

 } = ReactNative;

+const accessToken = 'pk.eyJ1IjoiZ21hcnR5IiwiYSI6ImNpdm1lanB1azAwNm8yb3A0OXZ6bmUxM2oifQ.i2HycDMBdPoatAB-ONR6FQ';
+Mapbox.setAccessToken(accessToken);


What are the rate limits for mapbox? We may need to flip to google-maps if we start entering paid usage tiers.

wilsonpage · 2016-11-18T14:57:01Z

lib/views/item-scene.js

+
+    // Distance in km
+    return R * c;
+  }


Can you shove this in utils/compute-distance.js?

wilsonpage · 2016-11-18T14:58:19Z

lib/views/item-scene.js

  },
+
+  distanceLabel: {
+    fontFamily: 'FiraSans-Book',


Use config.theme constants.

wilsonpage · 2016-11-18T15:00:15Z

Looks good @gmarty!

I haven't been able to test on device yet. Just some questions about the choice of map vendor and some code nits.

arcturus · 2016-11-18T16:23:22Z

Why is this close? Fat fingers? ;)

wilsonpage · 2016-11-18T18:17:34Z

Shit I always hit the wrong button!

wilsonpage · 2016-11-18T22:03:43Z

lib/views/item-scene.js

  Linking,
 } = ReactNative;

+const accessToken = 'pk.eyJ1IjoiZ21hcnR5IiwiYSI6ImNpdm1lanB1azAwNm8yb3A0OXZ6bmUxM2oifQ.i2HycDMBdPoatAB-ONR6FQ';


Let's put this in the config.js file. Maybe ask @samgiles or @arcturus about how we can protect keys. Prob not important right now though as this particular key doesn't hold much value.

Yeh, this key is fine - it wouldn't be so much if we paid for it - but still not the end of the world. Because we ship this key to the client, we can never really secure it, it's just not best practice to keep keys in the tree.

Will need to think about this, maybe we can use ci.tengam.org to store secrets required during builds, or use Travis's secrets mechanism.

Not sure what the best thing would be, let's figure that out at a later date.

wilsonpage · 2016-11-18T22:04:22Z

lib/views/item-scene.js


-  renderDistance(distance = 0) {
+  renderDistance() {
+    let distance = this.state.distance;


Why are we touching this.state? Is it meant to mean this.props?

Yes, it's a mistake.

wilsonpage · 2016-11-18T22:05:06Z

I'm getting this upon loading the ItemScene:

Not sure why we're touching this.state, perhaps it's a typo.

wilsonpage · 2016-11-18T22:11:15Z

lib/views/item-scene.js

+      distance = this.computeDistance(this.props.location.pos.coords);
+    }
+
    distance = Math.round(distance);


I think all this distance logic should be packaged up into a dedicated .getDistance() (pure) function. Takes required inputs and spits out a distance result.

wilsonpage · 2016-11-18T22:14:48Z

lib/store/actions.js

+    return (dispatch) => {
+      dispatch(locationUpdateStarted());
+      watchID = navigator.geolocation.watchPosition((pos) => {
+        dispatch(locationChanged(pos));


Can we be explicit and only pass lat/lon as that's all we're using right now. If we need other properties at a later date, we can expose them.

wilsonpage · 2016-11-18T22:16:22Z

lib/views/item-scene.js

+    if (typeof distance !== 'number' &&
+      typeof this.props.item.latitude === 'number' &&
+      typeof this.props.item.longitude === 'number') {
+      distance = this.computeDistance(this.props.location.pos.coords);


I don't think my device has got a location callback yet, so this is throwing.

wilsonpage · 2016-11-18T22:22:23Z

lib/store/reducer.js

+  location: {
+    pos: null,
+    isWatching: false,
+  },


A pattern I'm introducing with my pending redux patch is async values having status and value properties. The value property varies depending on the status.

{ location: { status: 'STARTED' | 'STOPPED' | 'ERROR'? value: null | { latitude: ..., longitude: ... } } }

Although I guess this is slightly different as the status doesn't directly map to a corresponding value. For example 'STARTED' doesn't mean that we have a value yet.

wilsonpage · 2016-11-18T22:26:09Z

@gmarty I'm having some difficulty running this patch, not sure what the differences are in our envs, but I'm not able to render the item-scene. I think in some cases we need to protect against the device not having a location value yet in render() function.

arcturus · 2016-11-22T11:49:40Z

config.js

      'privacy': 'https://trymagnet.org/privacy/',
      'feedback': 'https://trymagnet.org/#contact',
    },
+    mapboxAccessToken: 'pk.eyJ1IjoiZ21hcnR5IiwiYSI6ImNpdm1lanB1azAwNm8yb3A0OXZ6bmUxM2oifQ.i2HycDMBdPoatAB-ONR6FQ',


We will need to remove this key and add it during build time.

@gmarty sorry this isn't clear: settings is a namespace reserved for the SettingsScene. Please move your key up to the next parent.

arcturus · 2016-11-22T11:51:08Z

lib/views/item/item-detail.js

          style={styles.distanceImage}
-          source={require('../../images/item-scene-room.png')}/>
+          source={{
+            uri: 'ic_place_black_24dp',


do we need to indicate the dp?

Or can we just use 'ic_place_black' for devices with different dp, also does this work in ios?

arcturus · 2016-11-22T12:31:40Z

Looking gorgeous in ios, but unfortunately we still need to cover in this PR the ios integration.

wilsonpage · 2016-11-22T17:45:59Z

Looking gorgeous in ios, but unfortunately we still need to cover in this PR the ios integration.

@arcturus do you mean android?

arcturus · 2016-11-22T19:45:52Z

@arcturus do you mean android?

Right :)

wilsonpage · 2016-11-23T19:48:44Z

lib/views/item/item-detail.js

          style={styles.distanceImage}
-          source={require('../../images/item-scene-room.png')}/>
+          source={{
+            uri: 'ic_place',


Is there a reason why this image is not loaded using the regular require() mechanism?

It's not implemented in Mapbox yet. Plus I found working with image assets is really smooth both on iOS and Android. Using Material Design icons, I just had to slide the folders in relevant places.
And of course this support multi screen densities which is good.

wilsonpage · 2016-11-23T19:51:16Z

lib/views/item/item-detail.js

+            id: 'marker1',
+            annotationImage: {
+              source: {
+                uri: 'ic_place',


Again can we use require() to keep all our front-end image assets in one place?

wilsonpage · 2016-11-23T19:53:14Z

lib/views/item-scene.js

@@ -1,342 +0,0 @@
-/**


Where is this file, does this PR need a rebase?

wilsonpage · 2016-11-23T21:07:49Z

@gmarty I really don't want to put the emulator out of action on Android. This is because the emulator makes it possible to test the app across several android versions and mock locations when devs (me) aren't in London.

If the problem is related to the mapbox-native-gl version being out of date we have two options:

A. Override the transient com.mapbox.mapboxsdk:mapbox-android-sdk gradle dependency in our magnet-client's build.gradle so that we pull in the latest version.
B. Fork react-native-mapbox-gl and update the dependency. We can reference this fork in out package.json and submit a pull-request to the original. When the original fixes we can flip back.

wilsonpage · 2016-11-23T21:09:19Z

android/app/src/main/java/org/mozilla/magnet/MainApplication.java

        protected List<ReactPackage> getPackages() {
            return Arrays.<ReactPackage>asList(
                    new MainReactPackage(),
+                    new GoogleAnalyticsBridgePackage(),


Funky rebase issues?

wilsonpage · 2016-11-23T21:24:42Z

config.json

      "feedback": "https://trymagnet.org/#contact"
-    }
+    },
+    "mapboxAccessToken": "pk.eyJ1IjoiZ21hcnR5IiwiYSI6ImNpdm1lanB1azAwNm8yb3A0OXZ6bmUxM2oifQ.i2HycDMBdPoatAB-ONR6FQ"


Move this out of settings.

wilsonpage · 2016-11-23T21:30:14Z

ios/Magnet/Images.xcassets/ic_place.imageset/Contents.json

+    },
+    {
+      "idiom" : "universal",
+      "filename" : "ic_place_2x.png",


Isn't the convention ...@2x.png?

wilsonpage · 2016-11-23T21:32:33Z

I don't really like how our front-end image assets are now spread across three directories instead of one. I understand this is a limitation of mapbox API if you want to have custom marker images.

wilsonpage · 2016-11-23T21:37:07Z

Regarding android image assets:

All our android image assets currently have 12 different resolutions automatically generated by Android Studio.

res/drawable-hdpi
res/drawable-hdpi-v9
res/drawable-hdpi-v11
res/drawable-mdpi
res/drawable-mdpi-v9
res/drawable-mdpi-v11
res/drawable-xhdpi
res/drawable-xhdpi-v9
res/drawable-xhdpi-v11
res/drawable-xxhdpi
res/drawable-xxhdpi-v9
res/drawable-xxhdpi-v11

Your latest icon only has three different variations. For consistency I think all our icons should be available in the same variations.

wilsonpage · 2016-11-23T21:38:48Z

Happy for this to land once all comments addressed, most importantly the Android emulator issue. Nice work on this, it looks mega good!

wilsonpage · 2016-11-23T21:55:13Z

@gmarty please use this fix and keep all our front-end image assets in the lib/images/ directory. Merci! :)

wilsonpage · 2016-11-29T01:22:49Z

Closing this to continue work at #385

gmarty added the in progress label Nov 17, 2016

gmarty force-pushed the map branch 2 times, most recently from de41e5d to af888a9 Compare November 17, 2016 16:22

wilsonpage reviewed Nov 18, 2016

View reviewed changes

lib/store/actions.js Outdated

return {

type: 'LOCATION_CHANGED',

pos,

};

Copy link
Copy Markdown

Collaborator

wilsonpage Nov 18, 2016

Choose a reason for hiding this comment

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

Changes look spot on, nice one!

wilsonpage reviewed Nov 18, 2016

View reviewed changes

wilsonpage closed this Nov 18, 2016

wilsonpage removed the in progress label Nov 18, 2016

arcturus reopened this Nov 18, 2016

arcturus added the in progress label Nov 18, 2016

wilsonpage reviewed Nov 18, 2016

View reviewed changes

oteo assigned gmarty Nov 21, 2016

oteo added this to the Gate 0 - Sprint 8 [until Nov 18th] milestone Nov 21, 2016

gmarty force-pushed the map branch from af888a9 to 6559cfc Compare November 21, 2016 15:49

oteo added this to the Gate 0 - Sprint 9 [until Dec 2nd] milestone Nov 21, 2016

gmarty force-pushed the map branch from fa1ed82 to 4062bd8 Compare November 21, 2016 19:45

arcturus reviewed Nov 22, 2016

View reviewed changes

gmarty force-pushed the map branch 2 times, most recently from 7c80bd8 to 80358ce Compare November 22, 2016 15:03

gmarty force-pushed the map branch 2 times, most recently from b35cccc to 99f7c41 Compare November 23, 2016 15:57

gmarty mentioned this pull request Nov 23, 2016

Read the latitude and longitude from the scanner #324

Merged

gmarty added 2 commits November 23, 2016 19:39

Read the latitude and longitude from the scanner

b7929bf

Read the latitude and longitude from the scanner

03a244d

wilsonpage reviewed Nov 23, 2016

View reviewed changes

lib/views/item-scene.js Outdated

@@ -1,342 +0,0 @@

/**

Copy link
Copy Markdown

Collaborator

wilsonpage Nov 23, 2016

Choose a reason for hiding this comment

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

Where is this file, does this PR need a rebase?

Add a map for virtual beacons

3208561

gmarty force-pushed the map branch from 99f7c41 to 3208561 Compare November 23, 2016 20:04

wilsonpage reviewed Nov 23, 2016

View reviewed changes

oteo removed the critical for next release label Nov 23, 2016

wilsonpage closed this Nov 29, 2016

wilsonpage removed the in progress label Nov 29, 2016

Conversation

gmarty commented Nov 17, 2016 • edited by oteo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wilsonpage Nov 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilsonpage commented Nov 18, 2016

Uh oh!

arcturus commented Nov 18, 2016

Uh oh!

wilsonpage commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samgiles Nov 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilsonpage commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilsonpage commented Nov 18, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arcturus commented Nov 22, 2016

Uh oh!

wilsonpage commented Nov 22, 2016

Uh oh!

arcturus commented Nov 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmarty commented Nov 17, 2016 •

edited by oteo

Loading

wilsonpage Nov 18, 2016 •

edited

Loading

samgiles Nov 21, 2016 •

edited

Loading

wilsonpage commented Nov 23, 2016 •

edited

Loading