Skip to content

Conversation

@RubenKad
Copy link
Contributor

@RubenKad RubenKad commented Nov 8, 2024

Fixes #15404

PR was based on #15494

@RubenKad
Copy link
Contributor Author

RubenKad commented Dec 9, 2024

Is there a reason why the review takes a while? Did I miss anything?

@ahocevar
Copy link
Member

ahocevar commented Dec 9, 2024

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.

@RubenKad
Copy link
Contributor Author

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...

@RubenKad
Copy link
Contributor Author

@ahocevar
I tried to make a fork of #15494, but that was unfortunately not possible. Therefore, I did some small changes and here are some comments on the review comments of the older PR:

The other changes are exactly the same, so I hope this is enough information for the review!

@github-actions
Copy link

📦 Preview the website for this branch here: https://deploy-preview-16352--ol-site.netlify.app/.

Copy link
Member

@ahocevar ahocevar left a 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.

Copy link
Member

@ahocevar ahocevar left a 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!

@ahocevar ahocevar merged commit 644d80c into openlayers:main Jan 30, 2025
8 checks passed
@MoonE MoonE mentioned this pull request Mar 8, 2025
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.

Snap Interaction: dispatch event when not snapped anymore

2 participants