Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixes #8540 - Added "Mute All", Fixed "Mute Other Tabs", made "(Un)Mu… #8623

Closed
wants to merge 1 commit into from

Conversation

philkloose
Copy link
Contributor

Fixes #8540 - Added "Mute All", Fixed "Mute Other Tabs", made "(Un)Mute Tab" always present, changed Mute icon display behavior.

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

This PR contains several things, many of which weren't discussed in #8540. I probably shoved more stuff into this than I should have and I'm happy to have any and all ideas shot down -- I'm mainly doing this to familiarize myself with javascript and github.

  1. I added a "Mute All" menu item to the tabsToolbar context menu and verified that it strictly mutes instead of toggling. I did not implement a "master mute layer" proposed here Add new context menu to entire tab bar (including Reopen last closed tab) #8320 (comment) for a few reasons. One -- I didn't know how :). Two - I really couldn't come up with a use case for it and it would have been hard to convey the effect of the button to the user succinctly.
  2. The "Mute Other Tabs" tab menu item also suffered from the same issue of toggling, so I fixed that too.
  3. The "Mute/Unmute Tab" menu item was only conditionally showing when the tab was actively playing audio. This doesn't make sense to me since I want to be able to mute a tab before I begin playing a video. This behavior is identical to how Chrome works currently.
  4. If a tab is muted, the icon should show regardless of whether or not it is currently playing audio. I made this change. Again, this mimics Chrome's behavior.

Lastly, I would suggest like to suggest that the mute icon be made less ambiguous.
image vs image seems obvious until you realize that if the first one isn't present as a comparison that the second one simply looks like a speaker. The user doesn't know that it's missing the sound waves emanating from the speaker. I would propose an icon with the speaker slashed out.

…Un)Mute Tab" always present, changed Mute icon display behavior.
@philkloose
Copy link
Contributor Author

I just installed a Standard JavaScript formatting extension after this commit, so sorry for all the inconsistencies. I'll be sure to check/resolve them pre-commit going forward.

@bsclifton
Copy link
Member

@philkloose since your fix for #8303 was reverted, I'm going to close this PR for the moment. #8303 has notes with some next steps. If you're doing cherry picking, maybe you can include this fix in there too? 😄

Let me know if you need help

@bsclifton bsclifton closed this May 5, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab context menus should offer "Mute all tabs"
3 participants