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

Add symbol API #25

Merged
merged 1 commit into from
Mar 7, 2019
Merged

Add symbol API #25

merged 1 commit into from
Mar 7, 2019

Conversation

tobrun
Copy link
Collaborator

@tobrun tobrun commented Feb 9, 2019

WIP, not ready for review yet. Closes #11


A symbol is the Mapbox equivalent of Google Marker.
See more information in the style-spec here.

Most of the code in this PR is created using the code generator of https://github.com/mapbox/mapbox-plugins-android/tree/master/plugin-annotation. Once this API for symbol lands we can easily extend with with circles, fills and lines.

Current state is generating all code + showing a simple symbol:

ezgif com-video-to-gif 51

cc @yoavrofe

@tobrun tobrun self-assigned this Feb 9, 2019
@tobrun tobrun added this to the 0.0.2 milestone Feb 9, 2019
@tobrun
Copy link
Collaborator Author

tobrun commented Feb 17, 2019

ezgif com-video-to-gif 1

In the last two commits, I have added support for:

  • selecting the symbol in the example
  • correctly hooking into the symbol update mechanism
  • add support and default values for a couple of properties

The main thing that needs to be fixed before calling this feature ready is correctly handling state. Atm if you have update the rotation to say 270 degrees and change the opacity. It will reset the rotation to 0 instead of keeping the old value applied.

@tobrun
Copy link
Collaborator Author

tobrun commented Feb 24, 2019

Was able to track down where the symbol update is going wrong. Instead of providing default options as how Google Maps Flutter Plugin is doing, we need to provide default values during conversion time in the dart code. Reason is that the Mapbox Annotation Plugin is different as the Marker API on Google Maps SDK.

@tobrun
Copy link
Collaborator Author

tobrun commented Feb 24, 2019

Here is an example of it working:

asd

While it works for this specific case, I need to go through the whole API surface and make sure it works correctly.

@tobrun tobrun requested a review from yoavrofe February 24, 2019 20:37
@tobrun
Copy link
Collaborator Author

tobrun commented Feb 24, 2019

@yoavrofe with d5a664c it should be in a good condition for the first iteration of symbol/marker. Would you be able to review?

Copy link
Collaborator

@yoavrofe yoavrofe left a comment

Choose a reason for hiding this comment

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

Great work!
I attached some questions. I would be working on adding line annotataions as soon as this PR get merged.

@@ -48,7 +49,7 @@ public void setCompassEnabled(boolean compassEnabled) {

@Override
public void setCameraTargetBounds(LatLngBounds bounds) {
Log.e(TAG,"setCameraTargetBounds is supported only after map initiated.");
Log.e(TAG, "setCameraTargetBounds is supported only after map initiated.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not implemented yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes correct, delete for now?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should keep it, but I though you may think differently and wanted to point it out.

example/android/app/build.gradle Outdated Show resolved Hide resolved
@yoavrofe yoavrofe mentioned this pull request Feb 26, 2019
@yoavrofe
Copy link
Collaborator

Hi @tobrun , please see #29 . I would be glad if you could have a look.

@tobrun
Copy link
Collaborator Author

tobrun commented Mar 6, 2019

@yoavrofe thank you for the review and apologies for the delay (I was on holiday). I revisited this PR with your comments. Could you take another look?

Copy link
Collaborator

@yoavrofe yoavrofe left a comment

Choose a reason for hiding this comment

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

Cool! :shipit:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants