Implemented draggable and animated Marker and Annotation

[nikischin]

Implemented a draggable marker and an animated Annotation.

Updated storybook and support overview.

Generally speaking as the Mapkit JS Marker should inherit all features from Annotation, I was wondering if this would be also something we could consider in the implementation of those components.

[nikischin]

@Nicolapps any comments on this PR? :) Any chance of a review soon? Thank you!

I would like to further improve the library with some additional improvements, though would prefer not to have too many PRs open at the same time.

[Nicolapps]

Hey, thank you a lot for this PR! I’m sorry I didn’t see your PRs earlier.

I can review #38 once this PR is merged, and I will review #39 soon. What are the next things you would like to change in mapkit-react?

[nikischin]

Hi @Nicolapps, thank you so much for your review and all the suggestions you made. Fully agree with all of it!

Next steps related to this PR would be implementing access to the position of the draggable markers. Not sure if this should be implemented in this PR or we rather merge this one and do a new one for it?

Thank you!

[nikischin]

Hi @Nicolapps, as you can see, I decided, that it would make most sense to directly implement the Event parameters in this branch. Not sure if it is the best practice to use the same component as for the map, though I personally prefer to have as few redundant code as possible.

Happy to hear your opinion on this and get some further feedback!

[Nicolapps]

Thank you for the update! It’s indeed better to add the event parameter now so that we don’t need to change the API later. I will review your changes soon

[Nicolapps]

I had a look at your changes and I don’t think the event parameters are implemented correctly. I opened a PR with an alternative implementation + example here: 6f45234

What do you think? Does it do what you need?

[nikischin]

Hey @Nicolapps, thank you so much for your review and your Change request! I added my comments to your PR how I think about it. We can implement it like this but I feel like we are adding some unnecessary limitations with this implementation.

Generally speaking the main difference is that you are not re-using the same component as for the map which is totally fine (in my opinion adds redundant code, but that's some rather personal preference) and limits the events and params passed with the events.

It is your library, so we can totally implement it like you suggested, in the end you will be the one supporting it so it is totally fine for you to do the decision. If you would like we could also improve my solution in a way so that it is a bit more straightforward without the reuse of the map function, or some other modifications you would like to see on it.

[Nicolapps]

Hey @nikischin, thank you for your review! I added comments to the other PR to explain why I think that the API I used is more convenient for a React component. If this looks good for you, I can merge #44 and publish a new version of the package.

[nikischin]

Hi @Nicolapps,

I updated my branch on base of our latest discussion while doing especially the following modifications:

• implement your storybook example for maker
• add the missing event PropType definition
• pass only coordinate with the event as arg and not object. Removed the passed data object.
• removed the reuse of the forwardMapKitEvent function and implemented individual useEffects (with basically the same code)
• kept the coordinates for all event types instead of only the dragEnd

Let me know what you think about this implementation.

[Nicolapps]

Hi @nikischin, thank you a lot for these changes!

I have a last request: could you please remove the position parameter from the onSelect/onDeselect events, as they are not related at all to it? I looked again at your implementation and forwardMapKitEvent works well in Marker as well, but please move it to the util folder instead of exporting it from Map if you do so.

[nikischin]

@Nicolapps thank you so much!

Moving to the util folder is definitely better than exporting from the map, I agree!

Now I did a separate implementation for the marker and annotation, let me know if you prefer having it as one central function or 3 individual ones?

Also regarding the events, the events provide the following context by default: https://developer.apple.com/documentation/mapkitjs/map/handling_map_events#2993300

So the selection events basically provide the same context as the dragging events. Having the coordinates on selecting can really make implementations simpler and would be something I would prefer to have as it is already provided by the Framework. I can move it to a separate PR if you like though honestly it feels contraproductive first building a Interims solution to hen afterwards do another PR with the simplified solution we currently already have.

Does this make sense? Thank you!

[Nicolapps]

Thanks for your reply!

Now I did a separate implementation for the marker and annotation, let me know if you prefer having it as one central function or 3 individual ones?

It is fine to have one single function that takes a union type.

So the selection events basically provide the same context as the dragging events.

MapKit JS gives the whole marker as a parameter for every event, but this isn’t something that makes sense to do in a React library (especially because you already have everything you need in the same scope). For this reason, I would prefer to remove the parameter from select/deselect (we could even remove it from onDragStart as it is not necessary there, which might make the API clearer for its users, but I understand that it can be useful in some circumstances). Could you please update the PR so that we can publish a new version of the library as soon as it is merged?

[nikischin]

Thank you @Nicolapps !

I will write that utils function :)

I don't understand though how removing the position from the selection events would make the API more clear to use?

If I would want to have some moveable marker or annotation the user can place anywhere on the map and it does show nearby places in the callout using the other APIs or whatever function I have to provide some context, I could simply use the position provided by the event and would not need to keep track of the position in a separate state listening for the dragging or drag-end event, which possibly could save me also some unnecessary re-renders. I really don't think not having context in some of the events does add any clarity. Again, if we would not want to provide the position from the marker object provided we should remove it from all the events except the dragging event (which isn't yet even implemented), though I really don't understand why removing context would be any beneficial or add clarity.

[Nicolapps]

The goal would be to include the position in the event parameters only when it is needed because the event changes it. It is needed both for drag-end and dragging because there are use cases for both where we would need to get a new value through these events. This principle is used in many React libraries (e.g. react-map-gl).

Providing the location when it hasn’t changed would be confusing, because developers could infer that the event changes it, especially when provided as a single parameter like we are doing here.

Using the position obtained from outside will not cause a re-render, and useCallback can be used if there is a need to keep using the same event handler.

[nikischin]

Honestly saying, the whole discussion is super confusing.

In my opinion for a React wrapper for a JavaScript library, I would expect staying as close to the JavaScript library as possible. If you feel like adding other design principles/patterns from other libraries, feel free to do so as it is your repo, however, contributing for me is nearly impossible as you are constantly changing your opinion on the preferred behaviors.

The examples you just sent again implement the event parameters as object, including the original event if I understand it correct with the marker also as reference. So this is completely against what you previously told and also talking about only implementing those parameters in those events changing it (dragging and dragEnd) but then showing an example with all drag events (including dragStart) having the coordinates as parameter in my opinion is inconsistent.

I wrote my opinion any my preferred implementation already. Whatever you feel like the right thing, feel free to merge or leave the library as is. Discussing for a month on event parameters doesn't give me the satisfaction I would expect contributing to a project, so I might rather stick with a private library tailored to my needs, as this library doesn't seem to support those features I need (and would be willing to contribute) anytime soon.

Thank you so much for all your efforts!

[Nicolapps]

I appreciate your feedback and your contributions to the project. I understand that the discussion we had about the API has been frustrating for you, and I apologize for any confusion that may have arisen.

I understand your perspective on keeping the API close to the one that MapKit JS provides, but this isn’t the main design goal I had in mind when creating this library. I hope that your private fork will allow you to support your needs better. I will merge #44 which implements the API that I deem to be the most suitable for the library.

Once again, thank you a lot for your contributions and interest in mapkit-react!

[nikischin]

Hi @Nicolapps, thank you for your answer.

Could you please though add the design goals you had in mind to the contributing section of the readme, as it seems to be something super clear for you but not anything I was yet familiar with? Quite sure this would help anyone who would like to contribute to this repo.

Also it would be a great benefit to define the main goal of the library, as supporting MapKit JS as good as possible for my understanding is not the main goal and something else instead? Maybe it is rather a general map wrapper using the MapKit JS library? This again would help anyone not sure if this library is the right thing for their individual needs.

Thank you!

[Nicolapps]

Thank you for the suggestion! I updated the project README.

To clarify your question: the main goal of the library is to provide a convenient and straightforward API to use MapKit JS in React projects. Similarity to the APIs provided by MapKit JS itself is desirable when possible, but staying close to the principles of idiomatic React APIs is more important for this project.