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

[IMPROVE] Pinned Messages in E2E rooms #25325

Closed
wants to merge 29 commits into from

Conversation

yash-rajpal
Copy link
Member

@yash-rajpal yash-rajpal commented Apr 27, 2022

Proposed changes (including videos or screenshots)

Before

Screenshot 2022-04-27 at 11 08 59 PM

After

Screenshot 2022-04-27 at 11 09 15 PM

In Pinned Messages contextual bar -
Screenshot 2022-04-27 at 11 29 25 PM

If we don't have E2EE keys -
Screenshot 2022-04-27 at 11 08 44 PM

Issue(s)

Steps to test or reproduce

  • Create an end to end encrypted room.
  • Send message in the e2ee room and pin the message.
  • Pinned message should follow the e2ee rules.

Further comments

@yash-rajpal yash-rajpal requested a review from a team as a code owner April 27, 2022 18:12
@yash-rajpal yash-rajpal removed the request for review from a team April 27, 2022 18:12
@yash-rajpal yash-rajpal requested a review from a team as a code owner August 18, 2022 15:41
@yash-rajpal yash-rajpal marked this pull request as draft August 18, 2022 15:41
@yash-rajpal yash-rajpal marked this pull request as ready for review August 18, 2022 16:25
@codecov
Copy link

codecov bot commented Aug 18, 2022

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 53.70%. Comparing base (ff4e396) to head (832cd64).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff             @@
##           develop   #25325      +/-   ##
===========================================
- Coverage    55.59%   53.70%   -1.90%     
===========================================
  Files         2407     1112    -1295     
  Lines        52905    28341   -24564     
  Branches     10866     4000    -6866     
===========================================
- Hits         29415    15220   -14195     
+ Misses       20886    12291    -8595     
+ Partials      2604      830    -1774     
Flag Coverage Δ
e2e-api ?
unit 73.50% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@hugocostadev hugocostadev left a comment

Choose a reason for hiding this comment

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

Instead of passing the prop rid to be used to decrypt the message, try to use one of the existing hooks to get the rid , useUserRoom or useRoom

You can also detach the useRoom hook from RoomContext if needed

hugocostadev
hugocostadev previously approved these changes Aug 25, 2022
@ggazzo ggazzo added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels Aug 30, 2022
@yash-rajpal yash-rajpal added stat: needs QA and removed stat: ready to merge PR tested and approved waiting for merge stat: QA tested labels Sep 20, 2022
hugocostadev
hugocostadev previously approved these changes Sep 22, 2022
Copy link
Contributor

dionisio-bot bot commented Apr 12, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR is missing the 'stat: QA assured' label
  • This PR is missing the required milestone or project
  • This PR has an invalid title

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link
Contributor

dionisio-bot bot commented Apr 12, 2024

Looks like this PR is not ready to merge, because of the following issues:

  • This PR has conflicts, please resolve them before merging
  • This PR is missing the 'stat: QA assured' label
  • This PR is not mergeable
  • This PR is missing the required milestone or project
  • This PR has an invalid title

Please fix the issues and try again

If you have any trouble, please check the PR guidelines

Copy link

changeset-bot bot commented May 7, 2024

⚠️ No Changeset found

Latest commit: 832cd64

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@yash-rajpal
Copy link
Member Author

Closed in favor of - #32380

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

Successfully merging this pull request may close these issues.

5 participants