Skip to content
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

[7.0.0] API Design #1238

Open
nitaliano opened this Issue Jun 5, 2018 · 67 comments

Comments

Projects
None yet
@nitaliano
Copy link
Owner

commented Jun 5, 2018

For anyone that would like to leave feedback on what they would like to see in our upcoming major release please leave your feedback here.

@edolix

This comment has been minimized.

Copy link

commented Jun 6, 2018

Wow, looking forward to see this release 👍
I've found a strange bug described in #937 .

The upcoming release deprecates the PointAnnotation? I've read something related around issues or gitter channel.
Thanks!

@keenubee

This comment has been minimized.

Copy link

commented Jun 6, 2018

Do you plan to integrate routing?

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 6, 2018

@edolix PointAnnotation is going to be removed, AnnotationViews no longer exist in the next version of the Android SDK. There are plans to drop an Annotation component that wraps sources/layers and gives a similar API to PointAnnotation

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 6, 2018

@keenubee when you ask about routing are you talking about the nav SDK? That would be a separate react native SDK that we are debating if we should release or not

@kristfal

This comment has been minimized.

Copy link
Collaborator

commented Jun 7, 2018

@nitaliano

First of all, thanks a lot for your work on this wrapper. It's really good! Onto some wishes:

1) Camera API

Current state:
To fully use the camera API, a mix between setCamera and props on the <MapView> is required.

Ex: During user location tracking, the android app takes into account zoomLevel on the <MapView>, meaning that any zoomLevel applied via setCamera doesn't propagate to the user location tracking state. There are also quite a few other issues related to the camera that do exist currently.

Proposal:

  1. Go all in on a declarative prop-based camera API.
  2. Ensure that any combination of prop changes doesn't cause race conditions / ignored updates

Here is a proposal for camera related props:

  animated: {Function | Bool} – can either be set to true/false, or passed a function that takes two arguments: (currentCamera, nextCamera) => which in turn should return Boolean to indicate camera transition should be animated
  animationDuration: {Function | Number} – if animated is true, sets the duration of the animation. Can also be passed function that takes two arguments: (currentCamera, nextCamera) => which in turn should return a Number to indicate transition duration
  animationType: {Function | String} – If string, one of 'FlyTo' or 'JumpTo'. Can also be passed function that takes two arguments: (currentCamera, nextCamera) => which in turn should return a String to indicate transition type
  center: {Array} – camera center 
  zoom: {Number} – camera zoom
  bearing: {Number} – camera bearing
  pitch: {Number} – camera pitch
  bounds: {Array} – camera bounds. Setting this after setting center or zoom will override center and zoom and vice versa
  contentInset: {Array} – content inset of camera

With these props we've managed to make the camera fully declarative so setCamera, fitBounds, zoomTo and flyTo can be removed.

2) User location marker and camera API

Current state:

The user location is currently tied closely to the camera API, and there are quite a lot of significant differences between the iOS and Android implementation. The current implementation also leaves little room for flexibility in terms of selecting what marker to show and what camera state to be in.

Proposal:

  1. Go all in on a declarative prop-based location API
  2. Split up camera logic, camera follow logic and user location marker logic into separate props
  3. Write all user location marker and camera logic in Javascript to make iOS and Android completely equal (some logic in iOS can't be fully handled on Android at the moment, so I think its easier for devs to not have any subtile platform differences here)
  4. Always expose location, compass orientation (bearing) and vertical orientation (track) in the onUserLocationUpdate regardless of tracking mode or if showUserLocation is set to false

Proposal for user location marker related props:

  showUserLocation: {Boolean}
  userLocationMarkerType: {String} – one of: 'marker', 'markerWithViewOrientation', 'markerWithArrow', 'navigationArrow'. May also be able to pass in any valid sprite in the loaded sprite sheet or imageSource.
  showUserLocationAccuracy: {Boolean}

Proposal for user location camera related props:

  cameraFollowUserLocation: {Boolean} – if camera should follow the user's location
  cameraFollowUserOrientation: {String} – One of: 'none', 'heading', 'track'. Heading is compass and track is vertical speed orientation.
  cameraFollowUserLocationStopTrigger: {String} one of: 'anyGesture', 'panGesture', 'programmatic' and 'none'. Decides when the camera should stop following the user's location. 'anyGuesture' and 'panGuesture' will also stop on 'programmatic'.
  cameraFollowVerticalAlignment: {Number} – Number from 0 to 1 that declares the vertical fraction of the camera location. 0 being fully at top, 1 fully at bottom.
  cameraFollowHorizontalAlignment: {Number} – Number from 0 to 1 that declares the horizontal fraction of the camera location. 0 being fully to the left, 1 fully to the right.
  cameraFollowPitch: {Boolean} – If camera should have a set pitch during follow mode (declared separately)
  cameraFollowZoom: {Boolean} – If camera should have a set zoom level during follow mode (declared separately)
  cameraFollowPitchValue: {Number} – Pitch of camera during camera follow mode. 
  cameraFollowZoomValue: {Number} – Zoom of camera during camera follow mode. 

If cameraFollowUserLocation is set to false, the camera will return to whatever state is set by the regular camera props. If cameraFollowUserOrientation is set to none after being sett to one of the other alternatives, camera orientation will return to whatever state is set by regular camera orientation prop. The same logic applies for cameraFollowPitch and cameraFollowZoom.

Feel free to add / edit this list. Treat it as a lightweight proposal based on our experiences and not something very well defined.

@howardsj9

This comment has been minimized.

Copy link

commented Jun 7, 2018

Hi nitaliano thank you for starting this thread. Is a fix to the issue where custom icons(images) on the symbol layer blink/reload when zooming being addressed in the next release?

#1223

Thank you very much!

@sandroferrari

This comment has been minimized.

Copy link

commented Jun 8, 2018

Hi everyone !

  • I agree with @kristfal with ability to set custom marker for user location and camera API
  • Personnaly I'm waiting for #980 / #1198 😇

I'd like to take this opportunity to thank you @nitaliano and all of active contributors for your huge fantastic work, it's wonderfull !

Otherwise, I think the navigation SDK on react native would be a great idea because it would be the only one to propose a full navigation for react.

Thanks !

@ferdicus

This comment has been minimized.

Copy link

commented Jun 8, 2018

In addition it would be great to have the source/ layers play nice with animated values.
I would love to pipe in animated values into a shape and have it move smoothly from one coordinate to the next based on the animation timing.

<Mapbox.Animated.ShapeSource id="positionSource" shape={<point with animated lng/ lat values>}>  
  <Mapbox.Animated.SymbolLayer id="position" style={styles.icon}/>
</Mapbox.Animated.ShapeSource>
@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 8, 2018

@ferdicus that is something that will happen, I would like to build that into an Annotation component.

@kristfal love the ideas, this is something we will for sure explore and deserves it's own ticket to talk about implementation details.

@hampelm

This comment has been minimized.

Copy link

commented Jun 8, 2018

Thanks @nitaliano for all your hard work on this!

Layers not updating when URLs or IDs change (#1169) is a big sharp edge for us; would love that to be more react-y in a future release.

Would be nice to have long-press on feature layers (#1127). The generic version of this would be "all layer support all standard interaction types" but I haven't encountered any other situations where that's not the case.

@ferdicus

This comment has been minimized.

Copy link

commented Jun 9, 2018

Please overhaul the documentation if you find the time.
Things to consider:

  • examples on the documentation page
  • I'm a big fan of documentations that are one long page - makes it easier to find something and to compare
  • what does import Mapbox from '@mapbox/react-native-mapbox-gl'; itself expose - what methods are there, and what are they for?

To further elaborate on the last point:
Mapbox.geoUtils.makePoint - would be nice if something like this is mentioned in the new docs
Mapbox.setTelemetryEnabled - also this

Thanks :)

@bigsassy

This comment has been minimized.

Copy link

commented Jun 9, 2018

Will you be adding support for expressions? (i.e. https://www.mapbox.com/mapbox-gl-js/style-spec/#expressions)

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 10, 2018

@bigsassy yes expression support will be added. I already have it working on iOS and currently working on the Android side

@olliekav

This comment has been minimized.

Copy link

commented Jun 12, 2018

Thanks everyone for their work on this library, it's saved out project!

As mentioned by @kristfal decoupling the user location stuff would be great. @nitaliano I think you mentioned it on another issue about adding MapboxGL.locationManger and MapboxGL.UserLocation. I would love to move away from using the RN navigator.geolocation which seems really flakey and let this library handle all that.

We can't use showUserLocation right now as there is no way to click through the marker or add a custom one, so having to add a custom icon using navigator.geolocation which is painful.

@amitassaraf

This comment has been minimized.

Copy link

commented Jun 12, 2018

Great wrapper. Great library. My most requested thing would be a more explicit camera control so I can always know in the JS side the camera properties and only send them one way to the map. Meaning I know when the user touched the map and I know when the user zoomed the map and I don't have to wait for Native callbacks to arrive as it slows down updating the data that I am displaying on the map which translates to a bad user experience.

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 13, 2018

setNativeProps support will be added to 7.0.0, this is something I should have added long ago

@bigsassy

This comment has been minimized.

Copy link

commented Jun 13, 2018

Hey, @nitaliano. Do you have a broad estimate of when a 7.0.0 release would drop? Days? Weeks? Months? Too soon to tell? Thanks 👍

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 13, 2018

next month(July). I already have expressions working on both Android and iOS, now it's just adding the other features and fixing bugs

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 13, 2018

I roughly have a locationManager working, and am working on creating the UserLocation component

screenshot 2018-06-13 15 54 53

@olliekav

This comment has been minimized.

Copy link

commented Jun 14, 2018

@nitaliano This looks great, so the userLocation would act similar to a layer in terms of props (onPress, layerIndex, style etc).

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 14, 2018

Yup, and it would accept layers as children if you would want to customize it

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 14, 2018

This is the API I have setup for the locationManager

MapboxGL.locationManager.getLastKnownLocation()
MapboxGL.locationManager.addListener(listener)
MapboxGL.locationManager.removeListener(listener)
MapboxGL.locationManager.removeAllListeners()
MapboxGL.locationManager.start()
MapboxGL.locationManager.pause()
MapboxGL.locationManager.dispose()
@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 15, 2018

@kristfal I'm going to start taking a crack at making the camera declarative, I'll post back here with an API(which will be based around what you proposed) and we can get feedback on it.

@kristfal

This comment has been minimized.

Copy link
Collaborator

commented Jun 15, 2018

@nitaliano cool! We’ve made our own declarative wrapper in line with the suggestion above. It does really make things easy.

We did add one extra aspect tho: All location/zoom camera props also have an initial prop, eg: initialCenter and initialZoom. The reason for this being that in some cases, we want to set the camera initial state and not have to declare updated props on camera change.

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 15, 2018

@kristfal if you could share any code snippets or gists of what you and your team have done I would love to look it over and port over what makes sense to the SDK.

One thing I also want to make sure of is that we think of how this API will impact a navigation camera experience and interact with a potential RN Nav SDK.

@savv

This comment has been minimized.

Copy link

commented Jun 16, 2018

  • Support for testIDs to enable gray box testing. That is, I should be able to set a testID="pin-123" on an icon, and then find that icon from a detox test and click it.
@kristfal

This comment has been minimized.

Copy link
Collaborator

commented Jun 18, 2018

@nitaliano for the declarative camera API, you can have a look at:

https://gist.github.com/kristfal/9b1af1a381c1d57fe8dab1993cb1ad62

I included some of the helpers to give you an idea of how they work, however the gist isn't functional as it imports some stuff I've left out. In general, its a very light wrapper around what is.

We ended up with a camera API like this:

{
  bearing: {Number},
  bounds: [[{Number}, {Number}], [{Number}, {Number}]],
  center: [{Number}, {Number}],
  pitch: {Number},
  isInstant: {Boolean},
  bearing: {Number},
  zoom: {Number},
}

The same camera object could also be passed to the prop initialCamera. This camera API should probably be extended with at least an animationDuration key. There are some issues/bugs with this approach tho:

  1. Camera pitch doesn't work properly when applied via setCamera (iOS only issue)
  2. fitBounds lack options for changing pitch and bearing

We're getting user location from a separate react component, and user location layer is handled in separate map layer components.

We also ended up calculating the camera for user location track modes in a redux selector (straightforward except for vertical calculating offset). This responsibility should be moved into the react wrapper in your case. Let me know if you want some snippets on how we handle this.

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Jun 18, 2018

@kristfal thanks for the snippet!

This is what I'm loosely prototyping right now, to see what quirks I run into cross platform
screenshot 2018-06-18 13 53 46

All logic get's moved up to the JS layer, and the native layer will essential just be parsing a json config and applying the properties.

It is nice having a locationManager now, because I can just do and not have to worry about any other location needs

locationManager.addListener(listener)
@mattijsf

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

@nitaliano That camera API looks very nice! I'm not sure if you've seen it but I experienced some performance issues using initial camera bounds with the existing API's. Therefore I created this PR #1255. It would be great if this new Camera API would provide similar initial load performance :)

I think it will since it's layout (component) hierarchy is available upon creation of the MapView itself.

Edit: About the cross-platform quirks you're mentioning, I did ran into this one:
https://github.com/mapbox/react-native-mapbox-gl/pull/1255/files#diff-645e56b5e26606b8e46afa55273c4b22R356

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Oct 2, 2018

Posted my branch #1377 it's a huge api change, I wouldn't try to use this in any projects until we get this merged into master

@TheCodeDestroyer

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2018

I know this might be asking too much with your available time, but could you squeeze in custom layers?

@zugaldia

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2018

Hey all - it's been a while since we posted an update so I just wanted to let you all know that though we're moving slower than we wanted, we're still focused on a v7 release which we're targeting for December. Thank you all for your continued support and patience, I'll report back as soon as I have any more information to share.

@ferdicus

This comment has been minimized.

Copy link

commented Nov 7, 2018

That's great to hear @zugaldia.
Is @nitaliano still assigned to this project, or is it the two of you or are you taking over as @nitaliano has to take care of other responsibilities?

Thanks in advance

@zugaldia

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2018

@ferdicus v7 work is lead by @nitaliano :)

@atomheartother

This comment has been minimized.

Copy link

commented Dec 12, 2018

@nitaliano Is v7 still planned for December? We're starting work on a project using this component and of course if you're close to a new release we might want to work on the newer api...

@olivierstern

This comment has been minimized.

Copy link

commented Dec 12, 2018

@nitaliano Will V7 update native SDK for iOS Android to the latest versions? ;)

@zugaldia

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2018

hey all - as we wrap up the year I just wanted to give a quick update on the status of v7: our original plans have changed slightly but we're on track for a release on mid-January. we'll give further updates with the new year, meanwhile thank you all for your continued support and happy holidays!

@sandroferrari

This comment has been minimized.

Copy link

commented Jan 23, 2019

Hi guys ! Happy new year 🎉

What about v7 progression ? @zugaldia @nitaliano

@ferdicus

This comment has been minimized.

Copy link

commented Jan 29, 2019

Hey @zugaldia and @nitaliano, happy new year.

It has been a while since the last update and by now I feel this whole project feels a bit abandoned.
I'm thankful for the regular updates, however it is clear that this project needs more bodies.

Kinda hijacking this issue for a more general comment on the state of this repo.

This ticket was opened in June 2018.
As of writing this comment, there are 250 open issues, there were 4 commits to the 7.0.0 branch since it was opened and the activity in general seems to be a bit subdued. It seems like only @kristfal (thanks :)) is holding the fort here and he's not even working at mapbox.

I say this as someone who is loving what you guys are doing here.
And I'm super grateful for it, as we use it on a daily basis.

Just being a bit concerned is all I guess 😟

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Feb 7, 2019

Hey all - one more update from our side, thanks as always for your continued patience and support. We’ve thought a lot about the best way in which to continue supporting this project and community going forward and have decided it’s in the best interest of both to transition this project to be a community-driven project led by its original author, @nitaliano. This year the Mapbox mobile engineering team is focused on some large (and exciting!) new features in our native Android and iOS SDKs and we believe this decision brings the best path forward to guarantee this project continues being actively developed.

What this means in practical terms:

  • We’re looking for contributors! If you and your company depend on this project and would like to join the initiative, please reach out to @nitaliano and share more about your plans.
  • We’re moving this repo under @nitaliano’s account but nothing else changes: we’ll keep the git history, issue tracker, and code open source as it is today.
  • @nitaliano has been hard at work smoothing out the current v7 branch and you should expect a pre-release being pushed out in a couple of weeks.
  • Mapbox remains committed to prioritizing and fixing Android or iOS native issues on our Maps SDK that this community surfaces. Please keep opening issues on https://github.com/mapbox/mapbox-gl-native and tagging @zugaldia for visibility.

We’re grateful to all of you for making this project a vibrant community, we’re excited to see what this community builds next.

@ferdicus

This comment has been minimized.

Copy link

commented Feb 8, 2019

It feels weird when you talk about yourself in the third person :)

Great to have you back , still kinda worried about this though.

Not sure what happened to mapbox' commitment to this repo
that they are "giving away" the responsibility.

Are you still with Mapbox @nitaliano?

In any case, I'd be glad to contribute - even if it's just docs and issue triage.

Let me know when and where I can start :)

@mattijsf

This comment has been minimized.

Copy link
Contributor

commented Feb 8, 2019

I'm sad to see that the official support from Mapbox stops for react-native-mapbox-gl. At the same time I feel that the support from the community is an opportunity to get this project the boost it desperately needs in terms of quality / updates. To name one, the native dependencies are already far behind: iOS: 3.7.8 / May 7, 2018 and Android: 5.4.1 / Feb 9, 2018. Many native bugs have been fixed and features have been added that are currently unavailable to the react-native community.

We depend on this project and would like to contribute to this project @nitaliano

Edit:
I have to mention that apart from the contributions we did earlier last year (with a few PRs still pending), over the past months we have considered contributing to this project further. One of the important tasks would have been updating the native dependencies. However, like @kristfal mentioned below we were also held back by the pending 7.0.0 release / API changes.

So I'd say, get it out there and lets continue working on it! 👍 💯

@gvenk

This comment has been minimized.

Copy link

commented Feb 8, 2019

Sad to see the official support go away. @nitaliano. Keep up the good work! Hope this project will succeed as a community-driven project.

(and also curious if you are still with Mapbox @nitaliano)

@bigsassy

This comment has been minimized.

Copy link

commented Feb 8, 2019

I also have a little bandwidth each week to contribute 👍

@kristfal

This comment has been minimized.

Copy link
Collaborator

commented Feb 9, 2019

Hey guys! My company depends quite heavily on this repo. We’ve been on the fence for a while, not contributing as much as we could, due to the (ever so) imminent release of 7.0.0.

Moving forward, we’d be happy to bring more time and energy into it. It’s a bit sad that Mapbox officially drops support, but it’s great to hear that @nitaliano will still be here.

Thanks @nitaliano, and looking forward to working more on this in the future.

@sfratini

This comment has been minimized.

Copy link
Collaborator

commented Feb 11, 2019

@nitaliano Counts us in too! I sent you a private message. We use Mapbox in several projects and more in the future so we are invested in this.

I also think RN needs some AR love too. We woud like to integrate AR but I can only find Unity examples.

@maieonbrix

This comment has been minimized.

Copy link

commented Feb 13, 2019

Hello everyone :)

I would love to see the PointAnnotations api, or any other api providing us with the ability to put markers on the map, to be simplified like what we found in react-native-maps :

<Marker coordinate={{}} />

Also a better documentation.

I am open to help !

Thank you for you great work @nitaliano

@alexiri

This comment has been minimized.

Copy link

commented Feb 17, 2019

@zugaldia when will this repo be moved to @nitaliano's account? The sooner that happens, the sooner he can add more contributors and we can get the ball rolling again.

@nitaliano

This comment has been minimized.

Copy link
Owner Author

commented Feb 19, 2019

I'm assuming it will take a week or two, I no longer have admin access to the project so we'll just need to sit and wait until it happens.

@ferdicus

This comment has been minimized.

Copy link

commented Mar 4, 2019

@zugaldia tear down this wall!

(to prevent any confusion - I'm quoting this guy: https://en.wikipedia.org/wiki/Tear_down_this_wall!)

@ferdicus

This comment has been minimized.

Copy link

commented Mar 7, 2019

@agius can we get an intro? :)

@agius

This comment has been minimized.

Copy link
Collaborator

commented Mar 7, 2019

Hi @ferdicus ! Nice to meet you. I'm Andrew, from the Security team at Mapbox. I've sent @nitaliano an invitation to transfer the repo to him, just waiting until he has a free moment to handle that.

Github is a little strange in that it won't let you transfer a repo from an org to another org (unless you are an admin of both), or from an org to an individual (besides yourself). So that's why I've got the hot potato at the moment 😸

@zugaldia

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 2019

We are live! The transfer successfully happened a few hours ago. Thank you @agius and @nitaliano for coordinating the logistics.

image

@awdhootlele

This comment has been minimized.

Copy link

commented Mar 26, 2019

Hi, Thank you for this amazing library. I was looking for a feature to drag and drop the marker on the map. I am using ShapeSource + SymbolLayer to render a marker on the map view and I want the ability to drag the marker around and drop it to another location on the map.
If its already implemented, I would love to see some documentation on this.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.