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

feat: Pressable Group Updates #927

Open
wants to merge 5 commits into
base: release/2.0.7
Choose a base branch
from

Conversation

alexrisch
Copy link
Collaborator

Added handling when pressing a display name in the group updated messages

@alexrisch alexrisch requested a review from a team as a code owner October 4, 2024 18:16

// Group Updated Message
group_name_changed: '{{name}} changed the group name to "{{newValue}}".',
group_member_joined: "{{name}} joined the conversation",
Copy link
Member

Choose a reason for hiding this comment

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

@covrter This string has always felt that it's missing an explanation of how they got into the group. I think WhatsApp/Telegram/Signal all use variations of "{{inviter}} added {{joiner}}" for direct adds and "{{joiner}} joined via invite link". Should we do this too?

Copy link
Collaborator

@covrter covrter Oct 4, 2024

Choose a reason for hiding this comment

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

I think that's valuable to include the inviter, because it communicates trust — do we have the data handy?

It does again raise the question of when the "join" happens — if I'm added by a non-consented contact "badboy", I would not expect to see "badboy added andrew" until I consent to join the convo. I might expect for "badboy invited andrew" to appear in the chat, and then when I consent "andrew joined" to appear. (This creates accountability for inviting people in, bc the group can see it.) Does that feel right?

Either way this is its own task :)

Telegram example for my future reference
image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do have the information readily available for who added who (whom?)
But the other members of the group do not know if/when a different user "approves" (currently labeled as joins) the group

Copy link
Member

Choose a reason for hiding this comment

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

just riffing here, and this would be out of scope for now, but could the approval consent message be presented as a group update message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you mean the consent popup? "Do you want to join this group?"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It triggers a consent message, but that's on a different topic that other users in the group could not decrypt themselves (I cannot see your consent preferences)

The app can trigger some custom content type, but I think that's getting close to concerns with privacy

Copy link
Member

Choose a reason for hiding this comment

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

Ah, right. What is the privacy concern with using a content type message?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exposing consent of a group to the entire rest of the group.
If you "accept" a group, and then the app sends a message to tell everyone you have consented to the group might be a little weird

Unless we avoid showing the Group Updated messages and only show the Converse Group Consent message

Copy link
Member

Choose a reason for hiding this comment

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

I'm probably missing something, but that seems desirable. Group membership is already public to group members, and group consent creates a useful division between "invited" and "joined". We should check how other apps handle.

/cc @covrter

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah we would need to be mindful of how we are labelling these things

If Converse is relying on that, we need to run through all previous messages to see who's "consented" to the group, which will likely not be a complete list unless you are the creator of the group

@thierryskoda
Copy link
Collaborator

I just want to ensure these UI/UX changes are P0 or maybe P1, since we’ll redo most of it later with the design system.

Also, the tests will likely break after refactoring, making things harder to refactor. So my suggestion would be to not do any tests for now.

@alexrisch
Copy link
Collaborator Author

@thierryskoda I'll stop with component tests, but we need to find a balance between new features and introducing the design system

@alexrisch alexrisch closed this Oct 7, 2024
@alexrisch alexrisch reopened this Oct 7, 2024
@alexrisch alexrisch marked this pull request as draft October 7, 2024 18:19
Alex Risch and others added 5 commits October 15, 2024 11:24
Added handling when pressing a display name in the group updated messages
Fixed tsconfig
Added util to create text styles
Updated Chat Group Updated message to match design system
Added ParsedText component
@alexrisch alexrisch marked this pull request as ready for review October 15, 2024 18:25
@alexrisch
Copy link
Collaborator Author

@thierryskoda Updated with the design system:
Simulator Screenshot - iPhone 15 - 2024-10-15 at 14 20 51
I think we'll need to make a few updates to the presets to handle this though

@alexrisch alexrisch changed the base branch from main to release/2.0.7 October 15, 2024 20:02
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.

4 participants