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

Subscription audio notification #5366

Merged
merged 26 commits into from
Feb 8, 2017
Merged

Conversation

marceloschmidt
Copy link
Member

@RocketChat/core

Closes #171
Closes #71
Related to #1268

image

image

image

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-5366 December 29, 2016 12:06 Inactive
@marceloschmidt marceloschmidt temporarily deployed to rocket-chat-pr-5366 December 29, 2016 12:16 Inactive
Copy link
Member

@rodrigok rodrigok left a comment

Choose a reason for hiding this comment

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

Translation is missing
captura de tela 2017-01-03 as 17 32 31

The custom sound per room is not working, it is getting the sound of the current opened room for all notifications and not the sound for the room of the notification

Copy link
Member

@rodrigok rodrigok left a comment

Choose a reason for hiding this comment

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

Translation is missing
captura de tela 2017-01-03 as 17 32 31

The custom sound per room is not working, it is getting the sound of the current opened room for all notifications and not the sound for the room of the notification

@engelgabriel engelgabriel modified the milestones: 0.50.0, 0.49.0 Jan 11, 2017
@graywolf336 graywolf336 mentioned this pull request Jan 16, 2017
@engelgabriel engelgabriel modified the milestones: 0.50.0, Short-term Jan 24, 2017
@marceloschmidt
Copy link
Member Author

Translation is missing in Portuguese and in many other languages... translators do the work after our merges...

@marceloschmidt
Copy link
Member Author

I have fixed the issue with sounds being played based on currently opened room. Please review again @rodrigok ? Thanks.

Conflicts:
	server/startup/migrations/v078.js
@engelgabriel
Copy link
Member

@marceloschmidt you need to resolve the conflicts now

# Conflicts:
#	server/methods/saveUserPreferences.coffee
#	server/publications/subscription.coffee
#	server/startup/migrations/v083.js
# Conflicts:
#	server/startup/migrations/v084.js
@marceloschmidt marceloschmidt dismissed rodrigok’s stale review February 6, 2017 17:43

i18n cache problems; the translation key exists

@rodrigok rodrigok self-assigned this Feb 6, 2017
@engelgabriel engelgabriel modified the milestones: 0.52.0, Short-term Feb 6, 2017
@rodrigok
Copy link
Member

rodrigok commented Feb 7, 2017

@marceloschmidt the admin menu Custom Sounds is not working:
image

@rodrigok
Copy link
Member

rodrigok commented Feb 7, 2017

@marceloschmidt Fixed 😃

/* globals isSetNotNull, RocketChatTabBar */
Template.adminSounds.helpers({
isReady() {
if (isSetNotNull(() => Template.instance().ready)) {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for use this isSetNotNull?

Copy link
Member Author

Choose a reason for hiding this comment

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

CTRL+C, CTRL+V from adminEmoji :( should I remove it?

);

Template.adminSounds.events({
['keydown #sound-filter'](e) {
Copy link
Member

Choose a reason for hiding this comment

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

Why the selector is between brackets?

Copy link
Member Author

Choose a reason for hiding this comment

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

Likewise above

@engelgabriel engelgabriel merged commit 12f3870 into develop Feb 8, 2017
@engelgabriel engelgabriel deleted the subscription-audio-notification branch February 8, 2017 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to enable/disable notification sound/message Improve sound of new direct message
3 participants