-
Notifications
You must be signed in to change notification settings - Fork 694
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
support for custom annotation views #421
Conversation
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 Objective-C side looks good to me, other than a couple small things.
@@ -0,0 +1,50 @@ | |||
/** | |||
* Copyright (c) 2015-present, Facebook, Inc. |
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.
This copyright block is unneeded, right? Or is this adapted from somewhere?
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.
fixed
|
||
- (UIView *)view | ||
{ | ||
RCTMapboxAnnotation *marker = [RCTMapboxAnnotation new]; |
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.
Nit: [[RCTMapboxAnnotation alloc] init]
. new
works, but it’s an antipattern.
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.
fixed
@bsudekum or @dapetcu21, could you review the JavaScript side of this change? |
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.
Overall, looks extremely good.
let image; | ||
if (this.props.image) { | ||
image = resolveAssetSource(this.props.image) || {}; | ||
image = image.uri; |
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.
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?
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.
fixed
@@ -13,6 +13,7 @@ import { | |||
import cloneDeep from 'lodash/cloneDeep'; | |||
import clone from 'lodash/clone'; | |||
import isEqual from 'lodash/isEqual'; | |||
import Annotation from './MapboxAnnotation'; |
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.
Can we name the class MapboxAnnotation
or the file Annotation
to remove this mismatch?
}; | ||
|
||
const defaultProps = { | ||
onPress() {}, |
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.
looks like an unnecessary trailing ,
here
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.
fixed
Is there a plan to also implement this in Android? I would love to see this functionality work on both platforms. |
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) |
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. |
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. |
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. |
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
@1ec5 ok, added docs |
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. |
@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. |
I will submit other PR for that. |
@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. |
@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. |
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 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 |
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. |
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:
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 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 ? |
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
View-backed polylines and polygons aren’t supported by the native SDKs anyways. |
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
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
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. |
Yes, i saw this issue as well and did the same workaround.
|
Example of usage: