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

Added "Tap to remove" emoji in ReactionsBottomSheet #13908

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Sagar0-0
Copy link
Contributor

@Sagar0-0 Sagar0-0 commented Jan 13, 2025

First time contributor checklist

Contributor checklist

  • Device Realme GT Neo 3t, Android 14
  • Device Infinix Hot 20 Pro, Android 14
  • My contribution is fully baked and ready to be merged as is
  • I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

I understand there are a lot of lines changed but the overall flow is kept simple.

I suppose this is not a major feature release but a small improvement of existing functionalities and, thus does not need a rollout release. Let me know if I am wrong here.

  1. Call the MessageSender.sendReactionRemoval function to remove the icon from the bottom sheet.
  2. Getting the old reaction record object from the reactions table directly using messageId(efficient) rather than getting the message record and then the Reaction record from it.
  3. Added a new "Tap to remove" text in UI.
  4. Passing the Listener down to the ViewHolder and subscribing to the remove call to succeed.

I have tried to follow the coding standards used in the project, let me if anything needs to be improved here.

Output:

document_6066733776245887672.mp4

@Sagar0-0
Copy link
Contributor Author

Sagar0-0 commented Jan 14, 2025

I can also add Unit Tests for the changed classes if that is required before merging this feature. However, to add test cases we might need to do more refactoring and pass the ReactionsRepository a context object directly from calling fragments. (Only two sites uses this repo) For, refactoring we can go with some different approaches...

  1. Simply pass the object to the constructor of ReactionsRepository and change it in all call sites.
  2. Create a new ReactionRemovalRepository and pass it only to the ReactionsViewModel for this specific use case, this will not disturb any other flow. (Recommended as Single Responsibility principle).
  3. Pass the context object in the function call (Current implementation, difficult in testing).

I have added test-cases and they succeeded with both 1 and 2 approaches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant