Skip to content
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

FEAT(client,ui): Change tooltip of mute & deaf-buttons depending on their state #5141

Merged
merged 2 commits into from Jun 21, 2021

Conversation

ltoenning
Copy link
Contributor

@ltoenning ltoenning commented Jun 20, 2021

Previously the tooltips of the mute and deaf buttons (on main window) might caused confusion. The tooltips always showed: "Mute yourself" or "Deafen yourself" even if this user was already muted/deafen.

IMHO the tooltip should rather show exactly WHAT would happen if the user clicks on this button.

This commit adds the logic for setting the tooltip depending to its button state. It also introduces two new (translation) strings "Unmute yourself" and "Undeafen yourself".

Checks

Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

The changes LGTM 👍

For the commit format I would have two requests though:

  1. Please remove the changed *.ts files from your commit. Instead of bundling translation changes into the commit that introduces them, we prefer to do that in a separate commit. For that to work, please restore all *.ts files to their original state (git restore src/mumble/*.ts) and then run scripts/updatetranslations.py. That will take care of creating the necessary commit for you.
  2. Please also add the scope client to your current commit so that you end up with FEAT(client,ui). This makes changelog generation easier :)

@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature ui labels Jun 20, 2021
@ltoenning
Copy link
Contributor Author

Should be fixed :)

I wasn't sure about the .ts files as the pipeline didn't succeed without it. I first though squashing might be the better variant but now I agree separate commits are much cleaner!

@ltoenning ltoenning changed the title FEAT(ui): Change tooltip of mute & deaf-buttons depending on their state FEAT(client,ui): Change tooltip of mute & deaf-buttons depending on their state Jun 20, 2021
…heir state

Previously the tooltips of the mute and deaf buttons (on main window) might caused confusion. The tooltips always showed: "Mute yourself" or "Deafen yourself" even if this user was already muted/deafen. 

IMHO the tooltip should rather show exactly WHAT would happen if the user clicks on this button.

This commit adds the logic for setting the tooltip depending to its button state. It also introduces two new (translation) strings "Unmute yourself" and "Undeafen yourself".
@Krzmbrzl
Copy link
Member

Unrelated to this specific PR: Do you have a suggestion were to put instructions on how to deal with translations? We have the "issue" of contributors not knowing how we handle translations every now and then and I certainly don't blame anyone because we don't actually have it documented anywhere.
However I am wondering where we should put this in order for new contributors to actually find and read it ^^
Therefore as someone who just made their first contribution to this project: Is there a place we could have documented this where you would have found and read it? :)

@ltoenning
Copy link
Contributor Author

Good questions... First I skimmed through the README.md. Perhaps there a "Developer/Contributor information" section with some links to other .md or Github wiki pages?

@Krzmbrzl Krzmbrzl merged commit e4b6a12 into mumble-voip:master Jun 21, 2021
@Krzmbrzl
Copy link
Member

Thanks for your contribution!

And I'll see if I can find a nice spot in the main README for some contribution guidelines :)

@ltoenning ltoenning deleted the fix/tooltips branch June 21, 2021 20:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants