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

[NEW] Configurable Volume for Notifications #6087 #7517

Merged
merged 14 commits into from
Jul 27, 2017
Merged

[NEW] Configurable Volume for Notifications #6087 #7517

merged 14 commits into from
Jul 27, 2017

Conversation

lindoelio
Copy link
Contributor

@RocketChat/core

Closes #6087

image

@CLAassistant
Copy link

CLAassistant commented Jul 18, 2017

CLA assistant check
All committers have signed the CLA.

@lindoelio lindoelio closed this Jul 18, 2017
@lindoelio lindoelio reopened this Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@RocketChat RocketChat deleted a comment Jul 18, 2017
@lindoelio lindoelio closed this Jul 19, 2017
# Conflicts:
#	.meteor/packages
#	package.json
#	packages/rocketchat-ui/client/lib/notification.js
@lindoelio lindoelio reopened this Jul 19, 2017
# Conflicts:
#	.meteor/packages
#	package.json
#	packages/rocketchat-ui/client/lib/notification.js
@lindoelio lindoelio closed this Jul 19, 2017
@lindoelio lindoelio reopened this Jul 19, 2017
@lindoelio lindoelio changed the title [NEW] Configurable Volume for Notifications (#6087) [NEW] Configurable Volume for Notifications #6087 Jul 24, 2017
@rodrigok rodrigok added this to the 0.58.0 milestone Jul 24, 2017
@rodrigok rodrigok requested review from rodrigok and ggazzo July 24, 2017 14:14
@@ -397,7 +397,6 @@ class ModelUsers extends RocketChat.models._Base {
requirePasswordChangeReason
}
};

Copy link
Member

Choose a reason for hiding this comment

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

Please, do not introduce unnecessary changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. Fixed!

@@ -70,7 +70,6 @@ RocketChat.settings.addGroup('Accounts', function() {
type: 'boolean',
'public': true
});

Copy link
Member

Choose a reason for hiding this comment

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

Please, do not introduce unnecessary changes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. Fixed!

@@ -619,7 +619,7 @@ var jsc = {

switch (controlName) {
case 'pad':
// if the slider is at the bottom, move it up
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change these comments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry. I've struggled by my find-replace :-) Fixed!

@@ -872,6 +872,7 @@
"Nothing_found": "No se encontró nada",
"Notification_Duration": "Duración de la notificación",
"Notifications": "Notificaciones",
"Notifications_Sound_Volume": "Notifications sound volume",
Copy link
Member

Choose a reason for hiding this comment

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

This is not correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

.meteor/packages Outdated
@@ -173,3 +174,4 @@ yasinuslu:blaze-meta
deepwell:bootstrap-datepicker2
rocketchat:postcss
dynamic-import@0.1.1
fourseven:scss
Copy link
Contributor

Choose a reason for hiding this comment

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

We use only cssnext as CSS post-processor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. SCSS removed.

@@ -748,6 +748,7 @@
"Nothing": "asgjë",
"Nothing_found": "Asgjë për të gjetur",
"Notifications": "Njoftime",
"Notifications_Sound_Volume": "Notifications sound volume",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -58,7 +58,10 @@ this.visitor = new class {

// notification sound
if (Session.equals('sound', true) && msg.u._id !== Meteor.userId()) {
$('#chatAudioNotification')[0].play();
const audioVolume = Meteor.user().settings.preferences.notificationsSoundVolume || 100;
const audio = $('#chatAudioNotification')[0];
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use document.getElementById here, it's faster if you won't use jQuery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback. Fixed!

@@ -0,0 +1,112 @@
$shade-10: #04436a !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't use scss, just cssnext and less(we will remove all less styles in the future). You need to add your style on rocketchat-theme package and set your custom colors there on variables.js and colors.less files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your feedback and explanation. I've done as you suggested. Fixed!

* Input Range Slider
*/

.range-slider__range {
Copy link
Contributor

Choose a reason for hiding this comment

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

We already have classes for these colors, can you add those classes on your template?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -1,5 +1,6 @@
/*globals defaultUserLanguage, KonchatNotification */
import toastr from 'toastr';

Copy link
Contributor

Choose a reason for hiding this comment

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

can you remove that unnecessary change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -0,0 +1,72 @@
*,
Copy link
Contributor

Choose a reason for hiding this comment

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

we already have this selector on base.css file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

content: '';
}

::-moz-range-track {
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make a selector that work only on the slider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

border: 0;
}

input::-moz-focus-inner,
Copy link
Contributor

Choose a reason for hiding this comment

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

can you make a selector that work only on the slider?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}

.range-slider__range::-webkit-slider-thumb {
-webkit-appearance: none;
Copy link
Contributor

Choose a reason for hiding this comment

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

we already use autoprefixer on css build, so you can remove these browser prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

height: 20px;
border-radius: 50%;
cursor: pointer;
-webkit-transition: background 0.15s ease-in-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

we already use autoprefixer on css build, so you can remove these browser prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!


.range-slider__range {
-webkit-appearance: none;
width: calc(100% - (73px));
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use calc(100% - 73px)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

}

.range-slider {
margin: 0 0 0 0%;
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use margin: 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

border: 0;
border-radius: 50%;
cursor: pointer;
-webkit-transition: background 0.15s ease-in-out;
Copy link
Contributor

Choose a reason for hiding this comment

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

we already use autoprefixer on css build, so you can remove these browser prefixes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

let audio = $(e.currentTarget).data('play');

if (!audio || audio === 'none') {
audio = Meteor.user().settings.preferences.newMessageNotification || 'chime';
Copy link
Member

Choose a reason for hiding this comment

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

Meteor.user().settings or Meteor.user().settings.preferences can be undefined, can you check before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

return audio && audio.play && audio.play();

if (user && user.settings && user.settings.preferences) {
const newMessageNotification = user.settings.preferences.newMessageNotification || 'chime';
Copy link
Member

Choose a reason for hiding this comment

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

If there is no user.settings.preferences then there will not have notifications?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed!

@@ -58,7 +58,10 @@ this.visitor = new class {

// notification sound
if (Session.equals('sound', true) && msg.u._id !== Meteor.userId()) {
$('#chatAudioNotification')[0].play();
const audioVolume = Meteor.user().settings.preferences.notificationsSoundVolume || 100;
Copy link
Member

Choose a reason for hiding this comment

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

Can you check the properties here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Checked!

Copy link
Contributor

@karlprieb karlprieb left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution 👍

@rodrigok rodrigok merged commit 82b7845 into RocketChat:develop Jul 27, 2017
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.

None yet

4 participants