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

Reactions list modal improvements #81

Merged
merged 9 commits into from
Feb 2, 2024

Conversation

dsevillamartin
Copy link
Member

Changes proposed in this pull request:

  • Allow deleting individual post reactions in post reactions list
  • Fix emojis being too large in modal
  • Fix showing if user has voted on the post already

Reviewers should focus on:
This logic does not work for anonymous votes. It'd make sense to allow to delete an entire reaction type (I really hate the terminology being the same 😮‍💨), but I wasn't sure what API URL to use for that. As it stands already, it's not clear whether the endpoint is for a post_reaction or reaction ID... and since /posts/{id}/reactions returns post_reactions, I don't know whether the mismatch can be avoided at all.

Screenshot
image
image image

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
    • I haven't looked at tests at all. I know I could make them, but didn't feel like putting the work to learn & have the setup ready rn

@dsevillamartin
Copy link
Member Author

dsevillamartin commented Jan 13, 2024

Haven't implemented Pusher or tests. For Pusher, probably would want another event that just updates the count of the reaction type and have front-end handle if the user's reaction has been deleted... otherwise would require many events to be fired for deleting reaction types off posts.


image
image

@dsevillamartin dsevillamartin merged commit 3eb10da into master Feb 2, 2024
27 checks passed
@dsevillamartin dsevillamartin deleted the ds/reactions-modal-improvements branch February 2, 2024 19:28
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