-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
Lower hand #14851
base: master
Are you sure you want to change the base?
Lower hand #14851
Conversation
Hi, thanks for your contribution! |
03484de
to
efebe1e
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.
Great work so far! Left a few comments, let's discuss today.
// raisedHandTimestamp: participant.raisedHandTimestamp | ||
// }); | ||
|
||
queue = [ ...queue, { id: participant.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.
Why is this change necessary?
lang/main.json
Outdated
@@ -786,6 +786,7 @@ | |||
"newDeviceAction": "Use", | |||
"newDeviceAudioTitle": "New audio device detected", | |||
"newDeviceCameraTitle": "New camera detected", | |||
"nextToSpeak": "Your turn to speak might come soon, get ready", |
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.
Let's do the next to speak as a separate PR, since it's not related to lowering the hand.
@@ -1,8 +1,10 @@ | |||
|
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.
Please undo this.
@@ -147,6 +151,9 @@ export const FooterContextMenu = ({ isOpen, onDrawerClose, onMouseLeave }: IProp | |||
onClick: muteAllVideo, | |||
text: t('participantsPane.actions.stopEveryonesVideo') | |||
} ] } /> | |||
{raisedHandsQueue.length !== 0 && (<LowerHandButton |
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.
Is this check correct also when it's just the local user the one who raised the hand?
* | ||
* @returns {Function} A function that dispatches a notification action. | ||
*/ | ||
export function notifyToSpeak() { |
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.
Note for the next PR: don't put this in actions.web
since we'll want to reuse it for mobile too.
|
||
const handleClick = useCallback(() => { | ||
try { | ||
_isModerator && conference?.sendEndpointMessage( |
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.
This button should not be visible for non-moderators. Are we testing that?
@@ -19,6 +20,8 @@ MiddlewareRegistry.register(store => next => action => { | |||
/* onAllow */ () => APP.store.dispatch(setCameraFacingMode(data.facingMode)), | |||
/* initiatorId */ participant.getId() | |||
)); | |||
} else if (data?.name === LOWER_HAND) { |
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.
Check that the sender is a moderator.
bad19c7
to
5adaadf
Compare
Dear reviewer,
I have implemented functionality to lower a participant's hand and lower all hands for the meeting moderator. There are also some remaining code sections related to creating notifications for the next speaker. I have commented out notification-related listeners as these features are not yet fully implemented. I look forward to receiving your comments and advice on these updates.
All the best,
Mengyuan