-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Add Unsnap Event #16352
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 Unsnap Event #16352
Conversation
|
Is there a reason why the review takes a while? Did I miss anything? |
|
Sorry for not getting back to you earlier. Reviews are volunteer work, and it would be easier to see which review comments were already addressed in the original pull request (#15494) rather than having to review it all over in a new pull request. So if possible, please start from a fork of the branch in #15494, or at least provide some comments here referencing the review comments there, so it is easier to catch up with the changes. Also, please use English as language for commit messages. |
|
Thank you for the response, As the original PR was a year old and the changes to the code were quite minimal, I thought it would be best to make a new PR based on the current main branch of Openlayers, so that it is already up-to-date. And I thought after mentioning the previous PR with all the comments, which gave enough context. For the commit messages in a non-english language, that was my bad... |
|
@ahocevar
The other changes are exactly the same, so I hope this is enough information for the review! |
|
📦 Preview the website for this branch here: https://deploy-preview-16352--ol-site.netlify.app/. |
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.
Thanks for the explanations, @RubenKad, now I was able to remember and properly review. The changes look good! Could you please also add a test, to avoid future regressions on this? Also, please run npm run lint -- --fix to apply the Prettier fixes.
ahocevar
left a comment
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.
Thanks a lot @RubenKad for picking this up and finishing it!
Fixes #15404
PR was based on #15494