Skip to content
This repository has been archived by the owner on Sep 27, 2022. It is now read-only.

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

cpettet
Copy link

@cpettet cpettet commented Nov 22, 2021

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
Copy link

@austinwu13 austinwu13 left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link

@maikthomas maikthomas left a comment

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?

src/js/controllers.js Show resolved Hide resolved
@v-kpheng
Copy link
Collaborator

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

@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
Collaborator

@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
Copy link

maikthomas commented Nov 23, 2021

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
Collaborator

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

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