-
Notifications
You must be signed in to change notification settings - Fork 8
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
base: main
Are you sure you want to change the base?
Conversation
Thank you for your draft PR! I have a few remarks about the proposed APIs:
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. |
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 |
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?