HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Notifications dropdown with TinyScroll

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
withnotificationstinyscrolldropdown

Problem

I'm relatively new to jQuery and am trying to learn best practices/design patterns with JavaScript. Came across this link and decided to try and put it to something useful.

So far I've made a notifications menu dropdown using TinyScrollBar where the user can remove an individual notification. Once removed, the height of the dropdown updates until there are no more.

Here is a demo of what I have so far.

I would like to know ways on how I can improve my JavaScript.

JS

```
var TEST = TEST || {};

/ Notifications /
TEST.notif = {};
TEST.notif._notifOpen = false;
TEST.notif._el = null;
TEST.notif._sum = 0;
TEST.notif._defaultVPHeight = 350;

TEST.notif.init = function(){

this._el = $('#notifications');

$('.notif-trigger').on('click',function(e){
TEST.notif.toggleNotifications();
return false;
});

// when you click outside the notification menu
// hide the notifications
$('html').on('click',function(e){
if( TEST.notif._notifOpen ) {
TEST.notif.hideNotifications();
}
});

// set initial notifications scrollbar height
this._el.tinyscrollbar( {size: TEST.notif._defaultVPHeight} );
};

TEST.notif.showNotifications = function(){
var $el = this._el,
$close = $('.notif-close'),
$li = $('.notif-list li a');

// show notificatoins and update scrollbar position
$el.show().tinyscrollbar_update();

// stop propagation
$li.on('click',function(e){ e.stopPropagation(); });

// remove list-item
$close.on('click', function(e){

// store
var liHeight, newH, overviewH;

// store individual list-item height
liHeight = $(this).parent().height();

// remove the list-item you clicked
$(this).parent().remove();

// store the new height TEST the viewport
newH = TEST.notif._defaultVPHeight - liHeight;

// get height TEST actual contents
overviewH = $('.overview').height();

// set new defaultH
TEST.notif._defaultVPHeight = newH;

// update scrollbar
$el.tinyscrollbar_update();

// once scrollbar

Solution

From a once over:

-
Review your comments, some of them just take up space and are not very valuable:

// update scrollbar
$el.tinyscrollbar_update();


  • Indentation ( this might be because of copy pasting ) is off in your code, making it hard(er) to read



  • TEST.notif is a mouthful, I would just go for notifications



  • _el, _sum, _defaultVPHeight should all loose their underscore, they dont make sense. Also I would go either for the Spartan e or the full out element.



  • TEST.notif._notifOpen -> why notifOpen, it's already part of an object called notif, just call it open



  • Use lowerCamelCase consistently: tinyscrollbar_update -> tinyScrollbarUpdate



Other than that, your code is well structured and easy to follow.

Code Snippets

// update scrollbar
$el.tinyscrollbar_update();

Context

StackExchange Code Review Q#26355, answer score: 2

Revisions (0)

No revisions yet.