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

Ensure correct this context for addMembership function to call the blur method correctly. Fixes #10555 #10560

Merged

Conversation

Deeptanshu-sankhwar
Copy link
Contributor

Fixes issue #10555

  • Pass the this reference explicitly to the addMembership function to ensure the correct context is available.
  • Resolved an issue where this was undefined due to implicit context binding in event handlers.
  • Updated acceptEntity to pass the triggering DOM element explicitly as a parameter.

Screen Recording 2024-11-27 at 6 25 10 AM

Comment on lines 174 to 176
if (domElement && typeof domElement.blur === 'function') {
domElement.blur(); // avoid keeping focus on the button
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a defensive fix, but since addMembership is only called from one place, I think it would be cleaner to move this blur call to the caller, or even replace this with a closure that blurs the element, which would explicitly be in scope:

.on('accept', acceptEntity)

As it is, if acceptEntity has to bail for some reason, it won’t blur the element, which might result in the menu getting stuck on screen.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion @1ec5 ! I completely agree that handling the blur() in a closure at the .on('accept') level would ensure the element is blurred reliably and keeps the UI logic separate from addMembership. I will updat the code to use the closure approach, which explicitly handles the blurring before calling acceptEntity.

@Deeptanshu-sankhwar
Copy link
Contributor Author

I've updated the PR with the updated code to use the closure approach, which explicitly handles the blurring before calling acceptEntity. This change aligns with the feedback and avoids potential issues with the menu getting stuck on screen. Let me know if there are any further adjustments you'd recommend! 🙌

Copy link
Collaborator

@1ec5 1ec5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and congratulations on your first contribution!

@1ec5 1ec5 merged commit e48aa3c into openstreetmap:develop Dec 1, 2024
3 checks passed
@Deeptanshu-sankhwar
Copy link
Contributor Author

Thank you very much @1ec5 ! Hoping to be more involved.

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.

Error adding way to new relation — can't access property "blur", this is undefined
2 participants