support for custom annotation views #421

aksonov · 2016-09-22T16:04:48Z

Example of usage:

import Mapbox, { MapView, Annotation } from 'react-native-mapbox-gl';


...
render(){
     <MapView><Annotation coordinate={{longitude:LONG, latitude:LAT}}>... custom view here ..</Annotation></MapView>
}

1ec5

The Objective-C side looks good to me, other than a couple small things.

1ec5 · 2016-09-22T16:22:50Z

ios/RCTMapboxGL/RCTMapboxAnnotation.h

@@ -0,0 +1,50 @@
+/**
+ * Copyright (c) 2015-present, Facebook, Inc.


This copyright block is unneeded, right? Or is this adapted from somewhere?

1ec5 · 2016-09-22T16:25:58Z

ios/RCTMapboxGL/RCTMapboxAnnotationManager.m

+
+- (UIView *)view
+{
+    RCTMapboxAnnotation *marker = [RCTMapboxAnnotation new];


Nit: [[RCTMapboxAnnotation alloc] init]. new works, but it’s an antipattern.

1ec5 · 2016-09-22T16:27:25Z

@bsudekum or @dapetcu21, could you review the JavaScript side of this change?

tmcw

Overall, looks extremely good.

tmcw · 2016-09-22T16:32:11Z

MapboxAnnotation.js

+    let image;
+    if (this.props.image) {
+      image = resolveAssetSource(this.props.image) || {};
+      image = image.uri;


It looks like a specified but invalid image will fail silently here. Does resolveAssetSource emit a warning, or should we emit a warning or exception here?

tmcw · 2016-09-22T16:32:37Z

index.js

@@ -13,6 +13,7 @@ import {
 import cloneDeep from 'lodash/cloneDeep';
 import clone from 'lodash/clone';
 import isEqual from 'lodash/isEqual';
+import Annotation from './MapboxAnnotation';


Can we name the class MapboxAnnotation or the file Annotation to remove this mismatch?

tmcw · 2016-09-22T16:33:06Z

MapboxAnnotation.js

+};
+
+const defaultProps = {
+  onPress() {},


looks like an unnecessary trailing , here

rmevans9 · 2016-09-22T17:46:27Z

Is there a plan to also implement this in Android? I would love to see this functionality work on both platforms.

aksonov · 2016-09-22T18:15:32Z

Thanks, all, fixed.

@rmevans9 Sorry, only iOS at this moment, but you could adopt it easily for Android by looking into react-native-maps Android part (i've studied iOS part, AIRMarker* files)

aksonov · 2016-09-23T12:18:59Z

I believe it could be merged and then improved gradually. Old annotations (not custom) should work also as before. Btw, i have very strange issue with custom annotations - it works stable on simulator, but sometimes custom annotations are not shown on device. Maybe it is bug with iOS SDK.

1ec5 · 2016-09-23T15:55:07Z

Btw, i have very strange issue with custom annotations - it works stable on simulator, but sometimes custom annotations are not shown on device. Maybe it is bug with iOS SDK.

If you can reproduce this in a straightforward, stripped down project that uses the iOS SDK in Objective-C or Swift but not React Native, please file a bug with the test case in the mapbox/mapbox-gl-native repository. Otherwise, React Native and RNMBGL do a lot of magic that makes it difficult to attribute a bug to the SDK with certainty.

aksonov · 2016-09-26T07:45:09Z

When it could be merged?

P.S. I've fixed issues connected with device - map could be not created during add/remove annotations so, i've added proper fix.

1ec5 · 2016-09-26T15:58:36Z

Are any changes needed to the API documentation? We should probably point out that this feature is only available for iOS at the moment.

Add custom annotation note

aksonov · 2016-09-26T16:04:03Z

@1ec5 ok, added docs

1ec5 · 2016-09-27T10:09:04Z

Thanks! 🎉

Android support for view-backed annotations is being added in #423, although that PR takes a markedly different approach. Instead of arbitrary React Native views, the developer specifies the usual annotation options (plus a few more) and those options are implemented using MarkerViews.

aksonov · 2016-09-27T10:13:45Z

@1ec5 I'm preparing some improvements for this PR, because now annotation could disappear after scrolling map away and return back. So each Annotation must have 'id' and be used as reuseIdentifier for views.

aksonov · 2016-09-27T10:15:59Z

I will submit other PR for that.

tlvenn · 2016-10-13T05:19:08Z

@1ec5 Is there any platform reason/restriction as to why the Android support must be different ?

I dont want to dismiss the work @mrooding has put into his PR to bring this feature to android but it seems to me that we should aim to have an api that works the same on both IOS and Android if possible.

mrooding · 2016-10-13T05:51:46Z

@tlvenn I can't agree more. As said in my PR, if anyone could give me some pointers on how to get started I'm more than happy to contribute to the Android solution.

tlvenn · 2016-10-13T07:22:40Z

Unfortunately I cant help you much on this particular issue :(

One thing that I would like to confirm however is that @aksonov implementation allows for dynamic custom annotation views ? Similarly to the annotations array, could we have a viewAnnotations array we could manipulate ?

One could have some dynamic children but it suddenly force your app to have a different flow than how you would do thing with the annotations array.

1ec5 · 2016-10-13T15:24:17Z

Unfortunately, I don't know enough about how either the Android SDK or React Native works to give a cogent answer to these questions. All I know is that the Android and iOS SDKs in principle support the same feature: representing point annotations (markers) as native views (marker views); like any ordinary annotation, they can be added, removed, and relocated dynamically.

tlvenn · 2016-10-14T01:40:22Z

I kinda also think that @mrooding approach to leverage existing entry point into the annotations is much better than introducing an entire new way to define some annotations.

Why not simply support the following shape for a view type annotation:

annotations: [{
  coordinates: [40.72052634, -73.97686958312988],
  type: 'view',
  data: {}, //any data you need to properly render the view
  renderer: func // renderer function that takes data as param and return a react element
}]

The view would simply delegate its render method to the renderer function passing the data object as param.

Having a single source of truth for annotations is especially important when it comes to adding support for feature such as clustering of annotations and the like. All annotations no matter their type should be in the annotation array imho.

It also force the annotation view components to be stateless which is probably a good idea and aligned to current annotation types which are all stateless by nature.

I also dont think it's necessarily a good idea to introduce support for nested elements with children unless we do it consistently such as being able to pass any annotation type as children if this actually deemed a good idea, which i am not convinced.

Being able to back annotation with a view is absolutely fantastic but I hope we take the time to think what is the best approach to expose this to the end user.

@bsudekum @dapetcu21 any though on this ?

1ec5 · 2016-10-14T02:15:20Z

All annotations no matter their type should be in the annotation array imho.

That’s reasonable. However, I think the rationale behind allowing annotation views to be arbitrary children was that there are lots and lots of things you can do with views (at least on iOS). Nothing’s stopping you from embedding a switch control or an arbitrarily nested view hierarchy inside an annotation view, if that’s what you want. Would it be possible to enable all that flexibility inside the existing annotations array without adding explicit support for every possible customization? (Not a rhetorical question; I don’t profess to know what the right answer is.)

pass any annotation type as children

View-backed polylines and polygons aren’t supported by the native SDKs anyways.

wafisher · 2016-10-21T15:40:00Z

First off, I want to say this feature is awesome. I ran into a slight issue using it, though.

React seems to want to layout the Annotation with Flexbox in a container the width of the screen. With no extra layout code I have the following:

<Annotation id='location' coordinate={{latitude:40.750, longitude:-73.980}}>
  <Icon name="my-location" size={20} />
</Annotation>

and React puts this in a container view but on the left side, which is not where that coordinate is on the map. The coordinate is in the center of the container view.

If I change the code to

<Annotation id='location' style={{alignItems: 'center'}} coordinate={{latitude:40.750, longitude:-73.980}}>
  <Icon name="my-location" size={20} />
</Annotation>

It at least sits in the center of the container view, where it should be:

Sorry the pictures aren't terribly useful since Xcode only shows the wireframe of the map view in the debug view hierarchy.

Anyhow, this fix works but maybe there's a better way. But it should be a good workaround for anyone who needs something now.

aksonov · 2016-10-25T18:22:02Z

Yes, i saw this issue as well and did the same workaround.
Don’t understand why it happened yet, looks like it is connected with React Views internals. Any help would be appreciated.

On 21 Oct 2016, at 17:40, Will Fisher notifications@github.com wrote:

First off, I want to say this feature is awesome. I ran into a slight issue using it, though.

React seems to want to layout the Annotation with Flexbox in a container the width of the screen. With no extra layout code I have the following:

<Annotation id='location' coordinate={{latitude:40.750, longitude:-73.980}}>

and React puts this in a container view but on the left side, which is not where that coordinate is on the map. The coordinate is in the center of the container view.

https://cloud.githubusercontent.com/assets/3514570/19604262/7d394a16-9782-11e6-94cf-63f48bb095e1.jpg
If I change the code to

<Annotation id='location' style={{alignItems: 'center'}} coordinate={{latitude:40.750, longitude:-73.980}}>

It at least sits in the center of the container view, where it should be:

https://cloud.githubusercontent.com/assets/3514570/19604338/d0565d7e-9782-11e6-9880-8bec4b6511cb.jpg
Sorry the pictures aren't terribly useful since Xcode only shows the wireframe of the map view in the debug view hierarchy.

Anyhow, this fix works but maybe there's a better way. But it should be a good workaround for anyone who needs something now.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #421 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/ABQpcQfqD1ppLvHiG7Dx9QcQldyjYFM0ks5q2NzUgaJpZM4KEEi2.

support for custom annotation views

17835e0

aksonov mentioned this pull request Sep 22, 2016

View-backed annotations #404

Closed

1ec5 reviewed Sep 22, 2016

View reviewed changes

tmcw reviewed Sep 22, 2016

View reviewed changes

review fixes

0901d19

1ec5 added the ios label Sep 22, 2016

layoutIfNeeded doesn't display annotation sometimes

eb18cd0

fix small issues

a89a62b

Update API.md

f65c96d

Add custom annotation note

1ec5 mentioned this pull request Sep 27, 2016

Add MarkerView annotation #423

Closed

1ec5 merged commit 4ef787d into nitaliano:master Sep 27, 2016

mrooding mentioned this pull request Sep 29, 2016

add reuseIdentifier usage for custom iOS annotations to fix scroll away disappearing #426

Merged

1ec5 mentioned this pull request Sep 30, 2016

Annotation z-index #430

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support for custom annotation views #421

support for custom annotation views #421

aksonov commented Sep 22, 2016

1ec5 left a comment

1ec5 Sep 22, 2016 •

edited

Loading

aksonov Sep 26, 2016

1ec5 Sep 22, 2016

aksonov Sep 26, 2016

1ec5 commented Sep 22, 2016

tmcw left a comment

tmcw Sep 22, 2016

aksonov Sep 26, 2016

tmcw Sep 22, 2016

tmcw Sep 22, 2016

aksonov Sep 26, 2016

rmevans9 commented Sep 22, 2016

aksonov commented Sep 22, 2016

aksonov commented Sep 23, 2016

1ec5 commented Sep 23, 2016

aksonov commented Sep 26, 2016 •

edited

Loading

1ec5 commented Sep 26, 2016

aksonov commented Sep 26, 2016

1ec5 commented Sep 27, 2016

aksonov commented Sep 27, 2016

aksonov commented Sep 27, 2016

tlvenn commented Oct 13, 2016

mrooding commented Oct 13, 2016

tlvenn commented Oct 13, 2016 •

edited

Loading

1ec5 commented Oct 13, 2016

tlvenn commented Oct 14, 2016

1ec5 commented Oct 14, 2016

wafisher commented Oct 21, 2016

aksonov commented Oct 25, 2016

		@@ -0,0 +1,50 @@
		/**
		* Copyright (c) 2015-present, Facebook, Inc.

support for custom annotation views #421

support for custom annotation views #421

Conversation

aksonov commented Sep 22, 2016

1ec5 left a comment

Choose a reason for hiding this comment

1ec5 Sep 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 commented Sep 22, 2016

tmcw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmevans9 commented Sep 22, 2016

aksonov commented Sep 22, 2016

aksonov commented Sep 23, 2016

1ec5 commented Sep 23, 2016

aksonov commented Sep 26, 2016 • edited Loading

1ec5 commented Sep 26, 2016

aksonov commented Sep 26, 2016

1ec5 commented Sep 27, 2016

aksonov commented Sep 27, 2016

aksonov commented Sep 27, 2016

tlvenn commented Oct 13, 2016

mrooding commented Oct 13, 2016

tlvenn commented Oct 13, 2016 • edited Loading

1ec5 commented Oct 13, 2016

tlvenn commented Oct 14, 2016

1ec5 commented Oct 14, 2016

wafisher commented Oct 21, 2016

aksonov commented Oct 25, 2016

1ec5 Sep 22, 2016 •

edited

Loading

aksonov commented Sep 26, 2016 •

edited

Loading

tlvenn commented Oct 13, 2016 •

edited

Loading