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

Implement visible portion features #50

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nikischin
Copy link
Contributor

@nikischin nikischin commented Oct 2, 2023

Implementing features for center, rotation, region and visibleMapRect. (See #49)

Still draft as I wasn't able to fully test the rotation yet, as either storybook or my browser is a bit buggy.

@Nicolapps however, could you possible do a quick review if the general implementation seems reasonable to you?

@Nicolapps
Copy link
Owner

Thank you for your draft PR! I have a few remarks about the proposed APIs:

  • A lot of the new properties are overlapping (region, camera distance, center, visible map rect). This can lead to situations where the same parameter is specified several times, and it’s not clear what’s supposed to happen in these cases. Maybe it could be clearer to limit the number of available properties, but provide a way to convert the values between each other. Or another option could be to edit the TypeScript types to make it clear which properties are not meant to be set at the same time (maybe with something like discriminated unions).
  • It’s not possible to do bidirectional bindings of these properties with the current APIs. For instance, it is possible to set the camera distance from the user’s state, but the user doesn’t know when the camera distance changes. They could listen to the region events, but they can’t access the new value of cameraDistance without using refs.
  • MapKit JS doesn’t always emit events with the latest region values when browsing the map. When implementing bidirectional binding for these properties, I’m concerned about issues caused by the mismatch between the latest state in MapKit and the one that the user received. It would be nice to create a few stories in Storybook to show how it works in practice.
  • For some of these properties, MapKit JS overrides the value provided with a more suitable value as soon as it is set. For instance, when setting region, MapKit JS will use a region that fits the map ratio, which is most often not the case for the provided value. This could lead to a difference between the value being set and the value being applied, which works well in an imperative API like MapKit’s but could cause issues in the declarative context of React.
  • Would you want to deprecate initialRegion, as your comment in the code seems to indicate? This could still be useful for cases where the user only wants to set the initial region and wants to let the user browse the map freely without having to manage bidirectional binding.
  • What happens if the user sets region to a constant? According to React’s semantic, we probably shouldn’t let the map move (or revert the change instantly after the user is done panning?). It could be confusing if the implementation lets the set value and the actual value drift. (This is an issue we already have with the current implementation of the marker position and selected attributes, but it’s probably less confusing in this case since the ways these values can be modified are more limited.)
  • regionUpdateAnimates seems like a nice API which can make a lot of sense if the implementation works as expected, even when there is some bidirectional binding being used.

In general, I’m not sure if we could make an API like this one work well with the limitations of React and MapKit JS. But I’d love to be proved wrong and see stories that show it in action!

Out of curiosity, do you have more details about your use cases that make controlling these properties through attributes instead of refs? Using refs worked well for my project where I let the user pan freely the map and sometimes need to animate a region change to a particular location, but I understand that it might be different in your case.

@nikischin
Copy link
Contributor Author

Hi @Nicolapps,

thank you so much for this valuable feedback! Those are all valid points which on most I haven't yet have a clear answer to give. I will think more about this in detail and give an update here.

Generally speaking, the ref solution would work. I just personally don't like refs so much and I might have to introduce further useEffects for updating the values instead of just being able to pass a state. Although, yes it shouldn't make too much difference in the implementation.

I am personally working with a bigger software development team (product not involving MapKit JS) and we try to avoid refs wherever possible, as they can be a mess in maintenance and can be hard to debug but this again depends a lot on what they are used for and how. And yes, this rather applies to React than Javascript, though still it would be prettier for the code not having to use state and refs and just to rely on state. https://react.dev/learn/referencing-values-with-refs#best-practices-for-refs

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

Successfully merging this pull request may close these issues.

None yet

2 participants