Skip to content
This repository was archived by the owner on Sep 10, 2025. It is now read-only.

OPENTOK-44484: replaced 'mute on entry' and 'disable mute on entry' buttons with a single toggle button - #147

Open
cpettet wants to merge 4 commits into
opentok:masterfrom
cpettet:OPENTOK-44484
Open

OPENTOK-44484: replaced 'mute on entry' and 'disable mute on entry' buttons with a single toggle button#147
cpettet wants to merge 4 commits into
opentok:masterfrom
cpettet:OPENTOK-44484

Conversation

@cpettet

@cpettet cpettet commented Nov 22, 2021

Copy link
Copy Markdown

What is this PR doing?

This PR is replacing the 'Mute on Entry' and 'Disable Mute on Entry' buttons with a single 'Toggle Mute on Entry' button that combines their functionality.

How should this be manually tested?

  • run the app
  • toggle mute on entry button
  • toggle mute on self/publisher
  • open new browser tab
  • confirm functionality is correct
  • toggle mute on entry button
  • open another new browser tab
  • confirm functionality is correct

What are the relevant tickets?

OPENTOK-44484

austinwu13
austinwu13 previously approved these changes Nov 22, 2021

@austinwu13 austinwu13 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM!

@maikthomas maikthomas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Just a small change in the error handling.
Also @v-kpheng isn't the wording on this kinda misleading?
Doesn't toggling 'mute on entry' mute everyone else already in the call here?

Comment thread src/js/controllers.js
@v-kpheng

Copy link
Copy Markdown

Just a small change in the error handling. Also @v-kpheng isn't the wording on this kinda misleading? Doesn't toggling 'mute on entry' mute everyone else already in the call here?

@maikthomas, MuteOnEntry comes from the spec. The side effect of toggling depends on its state.

image

@maikthomas

Copy link
Copy Markdown

@v-kpheng Yea spec wise I see where this comes from, it's just UX wise we have a button that says 'Mute on Entry' that suddenly mutes everyone in the call 🤷

@v-kpheng

Copy link
Copy Markdown

@v-kpheng Yea spec wise I see where this comes from, it's just UX wise we have a button that says 'Mute on Entry' that suddenly mutes everyone in the call 🤷

Please feel free to suggest a different approach :-)
Would renaming the buttons to "Session-level mute" be sufficient? Or, is more needed? Also, meet has a host of UX issues already.

@maikthomas

maikthomas commented Nov 23, 2021

Copy link
Copy Markdown

In terms of UI, given the way this works my personal preference would be

  • 1st button state: "Force mute all"
  • 2nd button state: "Disable mute on entry"
    Is this strange? yes, but so is the functionality underneath, we're basically doing two separate things with one method 🤷‍♂️

Another unrelated question:
What about the UI for other participants? We probably need to listen to the muteAll events and update UI for all participants.

@v-kpheng

Copy link
Copy Markdown

In terms of UI, given the way this works my personal preference would be

  • 1st button state: "Force mute all"
  • 2nd button state: "Disable force on entry"
    Is this strange? yes, but so is the functionality underneath, we're basically doing two separate things with one method 🤷‍♂️

Another unrelated question: What about the UI for other participants? We probably need to listen to the muteAll events and update UI for all participants.

Hmmm...looks like we need a holistic approach. @cpettet, @austinwu13, can you guys sync up w/ @maikthomas. Let's come up with a more complete DoD please. The one I suggested isn't sufficient. Sorry.

Once you guys have consensus on changes needed, then let's implement that please. Thanks!

@maikthomas

Copy link
Copy Markdown

Sounds good to me, I think my previous answer is incomplete, better to meet and discuss 👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants