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

Add subscribe/unsubscribe buttons to note pages #5346

Merged

Conversation

AntonKhorev
Copy link
Collaborator

Part 5 of #5283 that adds the button to subscribe/unsubscribe from a note.

image

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

This mostly looks good - my only question would be about error handling for subscribe/unsubscribe as the error text reporting has been moved to only happen for the comment button.

The equivalent code for changesets reports the error in all cases by the looks of it - you can certainly argue that putting the text under the comment box in that case is not ideal but it's probably better than not showing it and errors should be very rare.

@AntonKhorev AntonKhorev force-pushed the note-subscriptions-buttons branch from b88e3f7 to aca9bd2 Compare November 21, 2024 21:56
@AntonKhorev
Copy link
Collaborator Author

Enabled showing errors for (un)subscribe buttons.

I didn't want to do this because the most likely error is that you're blocked and your access to the api is removed. But we shouldn't block people from unsubscribing. I thought that maybe I'll allow that first.

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

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

Thanks - that looks good now.

@tomhughes tomhughes merged commit 257f467 into openstreetmap:master Nov 21, 2024
22 checks passed
@AntonKhorev AntonKhorev deleted the note-subscriptions-buttons branch November 22, 2024 00:29
@AntonKhorev AntonKhorev mentioned this pull request Nov 22, 2024
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.

2 participants