-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
AV moderation and muting improvements #15628
Conversation
5ad4cf4
to
a4bac9f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a drive-by. LMK if you want a full review.
dispatch(playSound(ASKED_TO_UNMUTE_SOUND_ID)); | ||
// hide any existing notification as we will show a new one | ||
// the case where first audio is approved and then video is approved, we avoid double notification | ||
dispatch(hideNotification(ASKED_TO_UNMUTE_NOTIFICATION_ID)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to hide it if you are just about to push a new one. If it has the same ID it will replace it.
56b77df
to
832f73b
Compare
@@ -11,7 +11,7 @@ import { shouldShowModeratedNotification } from '../av-moderation/functions'; | |||
import { setAudioMuted, setVideoMuted } from '../base/media/actions'; | |||
import { MEDIA_TYPE, MediaType, VIDEO_MUTISM_AUTHORITY } from '../base/media/constants'; | |||
import { muteRemoteParticipant } from '../base/participants/actions'; | |||
import { getLocalParticipant, getRemoteParticipants } from '../base/participants/functions'; | |||
import { getRemoteParticipants } from '../base/participants/functions'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see why do we need this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ups, that needs to be in the first commit. "fix(video-menu): When muting all skip local."
I can move it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it was failing lint and I put it in the wrong commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
When muting multiple participants always skip the local one for audio and for video.
When both audio and video is to be allowed, make the audio the first one to show nad video to stay in the 3-dots menu.
We were showing only one option in the notification that was allowing both at the same time. We add not 3 option, allow audio, allow video or both.
…on sticky. If the notification disappears, we don't have any other indication about this. We were not showing any notification if only video is allowed. Adds option to unmute audio or video, depend on what was allowed.
832f73b
to
e950ca0
Compare
No description provided.