Add MarkerView annotation

[mrooding]

MarkerView annotation support

I've added support for rendering marker views. I added all extra properties available in the MapBox API like rotation, alpha, visible and flat.

Improvements

I struggled quite a while with actually getting the Marker views to actually render properly. I eventually found out that by clicking on a normal point marker it did suddenly make the marker views appear on the map. I eventually was able to replicate the workaround used in the onMarkerClick method by forcing the map to relayout. I had to add the force in 2 different places to get my use case working:

1. Rendering a MarkerView on the initial load when the map is still null
2. Repositioning the MarkerView by changing the annotation coordinates

What would really help is if someone was able to explain what the actual bug is that requires the relayout. If I can fix that we can skip the force relayout in several places like I have in this PR now.

[1ec5]

The three properties above can be implemented trivially on iOS. If you don't feel comfortable doing that, would you mind opening an issue tracking that work? In the meantime, please add a note that these four properties are currently unsupported on iOS.

[mrooding]

If you could point me in the right direction (which file for example) I'd be more than happy to give it a try.

[1ec5]

Actually, it doesn’t look like this will be compatible with #421. In that PR, on iOS, you can use an arbitrary React Native view as an annotation. As I understand it, this PR uses view-backed annotations to implement a fixed set of options. Once it lands, you’d be able to implement these options as exported properties on RCTMapboxAnnotationManager/RCTMapboxAnnotation.

alpha, rotation, and visible would correspond to the view’s alpha, layer.transform (use CATransform3DRotate() on the transform to rotate the layer), and hidden (negated) properties, respectively. However, with the approach taken in that PR, I don’t think it’s even necessary to provide dedicated options for these effects – the developer can do that themselves the way they normally do for React Native views.

[1ec5]

Would it be possible to implement something similar for Android in this PR, with arbitrary children views as annotations?

[mrooding]

I've looked at #421 but I'm not entirely sure how to get started with this on Android. How would we be able to render any react component inside the children?

[1ec5]

The relayout issue sounds reminiscent of mapbox/mapbox-gl-native#5560.

/cc @dapetcu21

[mrooding]

About the relayout issue: when I ran into the issue for the first time I wasn't aware of the fix that was made in the on click handler and I accidently found out that the click made my view appear. However, before I found the fix for the issue I did set up a native Android project with the Android MapBox SDK and was also not able to reproduce the issue.

[corymsmith]

Is this still being worked on to be more in line with #421? I will probably need this for a client project, if so I'm willing to help out, just curious what the current status is.

[mrooding]

I'm still not sure how we would implement #421 on Android. The Android SDK has support for adding Markers, Polylines, Polygons and MarkerViews and I merely implemented the missing MarkerViews.

I don't know a lot about the iOS SDK internals and why they would differ from the Android SDK so some guidance from one of the maintainers would be preferred.

[1ec5]

Regrettably, I'm not very familiar with how these things work in React Native, but I noticed that https://github.com/airbnb/react-native-maps has a MapView.Marker component that lets you define custom marker view content. That component apparently works on Android too. Perhaps it could be a starting point for an implementation here that works across both platforms.

[brianjd]

Thanks for your efforts @mrooding.
@1ec5 Why is this such a low priority for Mapbox? People have been looking for it since #143. There is a reason why react-native-maps has 15x more downloads p/m than react-native-mapbox-gl.

[1ec5]

Why is this such a low priority for Mapbox?

Simply put, at this time, we currently have more than enough work on our plates developing the native SDKs for Android and iOS.

As for me personally, I simply don’t know enough about the Android SDK, Android in general, or React Native to contribute further to this discussion. All I know is that those who designed RNMBGL intended to expose the same API across both platforms. If it’s going to diverge between them for annotation views, it seems like that needs to be an intentional decision documented in API.md. Otherwise, someone would need to either implement iOS support for these properties (that awkwardly won’t work with the existing children feature) or remove the children feature.

/cc @tmcw @bsudekum

[brianjd]

If Mapbox doesn't see the value in dedicating the necessary resources to reach feature parity for an immensely popular development framework, I don't know what to say. The community has made the effort, meet them halfway.

[tmcw]

A repository being in the Mapbox organization seems to connotate different things to different people, and that's perfectly acceptable: Mapbox is, after all, a company, and does, after all, have employees. We've considered moving this repo off of the organization to avoid that connotation - then the lack of 9-5'ers playing support and daily maintenance roles would be interpreted as a naturally informal community rather than a blamefully absent corporate entity.

So far I've been rallying to keep this repository under the Mapbox org, so the brilliant folks here can easily contribute to it and answer questions when they have time. But it's becoming clearer and clearer that having /mapbox/ in the URL makes anything a product, no matter what we say in the readme. Being in the /mapbox/ repo apparently means that people will expect a 9-5 employee dutifully answering questions and reviewing PRs.

Some things in the Mapbox org are side projects - for instance, this one was Bobby's fun project, and many others are mine. We have considered the /mapbox/ org being an easy way to open up the contributor base to coworkers. We tried, for this project, to make its status clear in the README, and I've tried, in issues, to contribute where I could and lead discussions.

Expectations and assumptions are, in this case, inescapable: if we keep this repository in the Mapbox organization, people will expect it to be a product. @bsudekum, want to re-adopt this to your personal account, or should we move it to a specialized org?

[ddolheguy]

@tmcw As a new React Native developer (using this library) but an old MapBox customer (using the JS library) I completely understand the frustration and I think it disappointing that people expect anything at all from an open-source project even if they are paying for the service via the API.

I think what annoys me most about the complaints I read is that if someone has a problem with the progression of a feature or a bug, then why don't they get off they behind, dig into the code, even spend the time to learn what they need to about Java/Objective C and attempt to fix it. Even if they raised a partial PR with most of the fixes and asked ppl to help. Arn't we all developers and isn't that our job?

Which ever direction you take, I support and I really hope you and the amazing people who put all their hard work into this library don't loose faith in continuing this wonderful project.

[brianjd]

Though react-native-mapbox-gl may have started as a “side project” it now represents an opportunity for the greater Mapbox organization. As an organization, Mapbox has the best chance of recouping any development investment versus any individual external project. And ultimately could have ridden the popularity wave of react-native to greater market share in new and existing markets. So yes, Mapbox should have spearheaded initiatives (or in this case fully rallied behind community efforts) to ensure the implementation of certain features. In turn, this would have converted react-native-mapbox-gl into a catalyst for growth for Mapbox, rather than keeping it at arm's length as a play thing.

Jettison if you must, but I still believe you are squandering the opportunity.

[ghost]

@tmcw I don't really make contributions for open source (as most of the stuff I work on is either commercial or personal projects), however I have been using Mapboxgl a for variety of projects I have my own version with changes of this repository working with your latest versions of the native bindings (iOS and Android), supporting stuff like GeoJSON, react native views, layers, etc... (disclaimer: I am not a mobile engineer by trade so my android/ios code is not perfect by any stretch of imagination - Probably the main reason why I haven't made any pull requests thus far).

Since I will be investing on MapBoxGL( mostly through react, including reactJS) for the foreseeable future, I am happy to fork the repository, apply my changes and maintain it so that you guys don't have to worry about the constant criticisms from other developers. (It would also be a great start to get more involved in open source).

Lastly, if you guys are ok with it, it would be cool to still have @1ec5 and yourself providing some feedback and the occasional code review (assuming you have the time) in oder to make sure we have a react version which fully exposes all the features that the mapboxgl sdk has to offer.

For everyone else: If you are interested in giving me a hand please let me know.

Thanks.

Removing my senatorial hold on this PR, because I don’t want it to seem like Mapbox is holding up development on this feature. To be clear, my initial requested change in #423 (comment) was to document the fact that the new options are Android-only and to reconcile the Android MarkerView and iOS child view implementations as tail work.

[1ec5]

Lastly, if you guys are ok with it, it would be cool to still have @1ec5 and yourself providing some feedback and the occasional code review (assuming you have the time) in oder to make sure we have a react version which fully exposes all the features that the mapboxgl sdk has to offer.

@AlexLourenco, it would be wonderful to see RNMBGL approach the native SDKs in terms of features and stability. Those who’ve chimed in on this PR are best positioned to take the project forward.

To the extent I’ve been involved in this repository, it has been for the purpose of reaching out to developers using the Mapbox iOS SDK – like those here who are writing RNMBGL! – and making sure they understand how to use the SDK. The code review I originally gave here is along those lines: just making sure everyone is on the same page regarding the capabilities of the Android and iOS SDKs. I don’t think I can help much beyond that, but we’d be open to giving non-Mapboxers collaborator access to this repository if that’ll help the community take the project forward.

If I recall correctly, @dapetcu21 strove to unify the API between Android and iOS. However, if it really is infeasible to support a unified API for MarkerViews/annotation views, then yes, I can push the big green button. That difference between the two platforms just needs to be documented, that’s all.

[ghost]

Fair enough!

It is possible to unify the API's, as I have done so. My approach is not too far from this PR and the one that got merged for the iOS but both have limitations plus they don't account for things like live editing (adding / removing react views constructs on the fly and see the changes live on the screen). React has its own way to manage views and therefore special care needs to be taken to make sure views are disposed of correctly (and even I need to allocate some time to make sure there are no memory leaks on my approach).

I have just forked the project as is. I will put my changes up in the next couple of days and make a test app repo so that I can get some feedback in terms of correct functionality. I will bump a comment here once I'm finished doing so.

Regards.

[tsemerad]

I really would prefer this repo stay where it is. My experience working with this project and contributing to it has been fantastic. When I've submitted PRs, it has been invaluable receiving expert feedback from people like @1ec5. I totally understand that it is an experimental project right now, and that it's not a full fledged Mapbox "product", and I think that's fine - that's how most OS projects work. I just hope that a few confused or upset developers don't ruin it for everyone. My vote goes to keeping it where it is, but maybe adding more outside maintainers to the repo. I'm willing to review PRs, at least from the JS and documentation side of things.

Also, moving it outside of the Mapbox org I fear may result in people being less likely to submit PRs. I have queryRenderedFeatures implemented on Android on my fork, which I intend to PR soon, and my coworker just implemented runtime styling on both iOS and Android (which we'll PR after iOS runtime styling is out of beta). We look forward to receiving feedback on them from the likes of @1ec5, @bsudekum, @tmcw, and @dapetcu21, and I'm just a bit nervous that moving the repo might change that feedback loop.

[1ec5]

Now that #509 has landed, children views are supported as annotations/markers on both Android and iOS. I’m going to close this PR because it looks like #509 addresses the same use case. However, if there are use cases that remain unmet, please open separate tickets to track them. Thanks!