patternjavascriptMinor
Object Oriented Modular Closure
Viewed 0 times
orientedobjectclosuremodular
Problem
The following is some code I just wrote. I keep wondering as always what I can do to make this more efficient. Please help me make this code object oriented and if anyone could help me write automated or even non automated tests for my widget...
Also is this considered a closure? I don't think it is but I would love to know how to produce better quality/reusable code. Oh- bonus points for any corrections to my techniques used as well. I'm really trying to step my JS game up and I know it needs work...
PS: Here is the fiddle
Also is this considered a closure? I don't think it is but I would love to know how to produce better quality/reusable code. Oh- bonus points for any corrections to my techniques used as well. I'm really trying to step my JS game up and I know it needs work...
var ExplodedContentWidget = $('[data-widget~="exploded-content"]');
// search for existance of the exploded widget in DOM
if (ExplodedContentWidget[0]){
var ExplodedDefaultEl = $('[data-function~="default-state"]');
var ExplodedEl = $('[data-function~="exploded-state-1"]');
var ExplodedEl2 = $('[data-function~="exploded-state-2"]');
var CloseButton = $('.button-close');
ExplodedEl.hide(); // default exploded content is hidden
ExplodedEl2.hide();
CloseButton.hide();
ExplodedContentWidget.on('click',function(){
ExplodedEl.slideToggle('slow');
});
var FullScreenViewButton = $('button');
FullScreenViewButton.on('click',function(){
ExplodedDefaultEl.hide();
ExplodedContentWidget.addClass('exploded-fullscreen-state');
ExplodedEl2.slideToggle('slow');
CloseButton.fadeIn();
ExplodedEl2.on('click',function(){
ExplodedContentWidget.removeClass('exploded-fullscreen-state');
ExplodedDefaultEl.show();
ExplodedEl.toggle();
ExplodedEl2.toggle();
CloseButton.hide();
});
});
}PS: Here is the fiddle
Solution
From a once over:
-
This :
I would have addressed with a css class that hides these elements
-
Your code passes JsHint.com nicely, well done
-
Also is this considered a closure? Your inline functions for
- Do not use
data-function~="exploded-state-1", instead assign id's and search by id, this is the common ( and faster, better ) approach
- One single
varstatement with comma separated variables is considered better
- I would go for
if (ExplodedContentWidget.length){instead ofif (ExplodedContentWidget[0]){
- Variable names should follow lowerCamelCase so
ExplodedEl->explodedEl, the exception to that is for jQuery results so you could also use$exploded1
-
This :
ExplodedEl.hide(); // default exploded content is hidden
ExplodedEl2.hide();
CloseButton.hide();I would have addressed with a css class that hides these elements
- You indent the code after
var FullScreenViewButton = $('button');, dont do that. Consider using the jsbeautifier website
$('button');
-
Your code passes JsHint.com nicely, well done
-
Also is this considered a closure? Your inline functions for
.on('click' are closures.Code Snippets
ExplodedEl.hide(); // default exploded content is hidden
ExplodedEl2.hide();
CloseButton.hide();Context
StackExchange Code Review Q#46087, answer score: 5
Revisions (0)
No revisions yet.