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

Attach breakoutGroup to all remote and local participants on room_joined, new_client and breakout_group_joined signal events #471

Conversation

richtrwhereby
Copy link
Contributor

@richtrwhereby richtrwhereby commented Nov 13, 2024

Description

Add breakoutGroup to all participants when that data is received from the signal server in room_joined and/or breakout_group_joined events.

Related Issue:
#481 (Add breakoutGroup to all screenshare objects)

Checklist

  • My code follows the project's coding standards.
  • Prefixed the PR title and commit messages with the service or package name
  • I have written unit tests (if applicable).
  • I have updated the documentation (if applicable).
  • By submitting this pull request, I confirm that my contribution is made
    under the terms of the MIT license.

Copy link

changeset-bot bot commented Nov 13, 2024

🦋 Changeset detected

Latest commit: 392e210

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@whereby.com/core Minor
@whereby.com/media Patch
@whereby.com/browser-sdk Patch
@whereby.com/react-native-sdk Patch

Not sure what this means? Click here to learn what changesets are.

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

@richtrwhereby richtrwhereby force-pushed the richardtibbett/pan-1488-attach-breakout-group-to-local-and-remote-participants branch 2 times, most recently from 5fefa44 to f2890d6 Compare November 19, 2024 14:51
@richtrwhereby richtrwhereby marked this pull request as ready for review November 19, 2024 14:58
@whereby whereby deleted a comment from github-actions bot Nov 19, 2024
@richtrwhereby
Copy link
Contributor Author

/canary

@richtrwhereby richtrwhereby requested a review from thyal November 19, 2024 15:06
@richtrwhereby richtrwhereby changed the title Attach breakoutGroup to all remote and local participants on room_joined and breakout_group_joined signal events Attach breakoutGroup to all remote and local participants on room_joined, new_client and breakout_group_joined signal events Nov 19, 2024
Copy link
Contributor

🚀 The canary releases have been published to npm.

You can test the releases by installing the newly published versions:

yarn add @whereby.com/browser-sdk@0.0.0-canary-20241119150730
yarn add @whereby.com/core@0.0.0-canary-20241119150730
yarn add @whereby.com/media@0.0.0-canary-20241119150730
yarn add @whereby.com/react-native-sdk@0.0.0-canary-20241119150730

@richtrwhereby richtrwhereby requested review from a team and removed request for thyal November 20, 2024 08:32
thyal

This comment was marked as resolved.

…om_joined and breakout_group_joined signal events
@richtrwhereby richtrwhereby force-pushed the richardtibbett/pan-1488-attach-breakout-group-to-local-and-remote-participants branch from f2890d6 to 392e210 Compare November 20, 2024 11:48
@richtrwhereby
Copy link
Contributor Author

Nice, works well. I've tested and see that the breakout group is set correctly. However there's an inconsistency here:

When there's no breakout active and you join a room with the SDK, the local participant will have breakoutGroup: null in the redux state, but the remote client will have breakoutGroup: "".

If you join a breakout group with the remote client, and then leave again the remote client state will correctly be set to breakoutGroup: null. I don't think this will cause any issues practically, but we should be consistent.

You can add this line in remoteParticipants.ts, in the createRemoteParticipant function to fix it:

export function createRemoteParticipant(client: SignalClient, newJoiner = false): RemoteParticipant {
    const { streams, role, ...rest } = client;

    return {
        ...rest,
        breakoutGroup: client.breakoutGroup || null,
        ...

Thanks this was a good shout and fixed now (force pushed) 🎉

Copy link
Member

@thyal thyal left a comment

Choose a reason for hiding this comment

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

🍰 Nice!

@richtrwhereby richtrwhereby merged commit 8ec779d into main Nov 21, 2024
3 checks passed
@richtrwhereby richtrwhereby deleted the richardtibbett/pan-1488-attach-breakout-group-to-local-and-remote-participants branch November 21, 2024 11:53
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