-
Notifications
You must be signed in to change notification settings - Fork 68
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
v2.0 🎉 🎉 🎉 #130
base: master
Are you sure you want to change the base?
v2.0 🎉 🎉 🎉 #130
Conversation
Awesome stuff, well done @AnatoleLucet I also have a couple of questions:
|
Thanks for taking the time to review this whole pr 🙏
Yes, I'm planning on writing the doc in the readme and make a contributing.md file.
I was planning on writing tests cases mainly for the |
24e22cc
to
c0e80d3
Compare
Hello @AnatoleLucet, and thanks once again for the outstanding work on the v2. I spent some time reviewing the PR and left a few comments - please let me know if they make sense. Happy to merge and ship under |
|
It seems there's a little formatting issue on Thanks once again for the excellent work, feel free to merge into master in your own time. |
Hey @MicheleBertoli I fixed the CI and pushed a few small changes. I think you need to approve the PR for me to be able to merge it, but feel free to merge! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🙌
const map = useGMapsInstance(); | ||
const google = useGMapsSDK(); | ||
|
||
const polyline = React.useRef<google.maps.Polygon | null>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just notice this - I believe it should be const polygon
.
Double-check that events are working
@AnatoleLucet, sorry for putting this back in your queue. However, as I was working on e2e tests, I noticed that events don't seem to be working. From a quick look, it seems the issue is that the hook that adds the event listeners only runs on updates. I hope this makes sense, and happy to provide more context if valuable. |
Interesting. Good thing we're going to have test cases for this then 😅 |
No description provided.