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...
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();
});
});
}
1 Answer 1
From a once over:
- 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
var
statement 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');
<- This is code waiting to go wrong, what if you have more than 1 button. Give the button an id, and select on that idYour code passes JsHint.com nicely, well done
Also is this considered a closure? Your inline functions for
.on('click'
are closures.
-
\$\begingroup\$ Thanks, I appreciate the help! The button was generic on purpose good point though! That would be a production nightmare haha? Any luck on how I could make this function pass object oriented? \$\endgroup\$NicholasAbrams– NicholasAbrams2014年04月02日 19:24:51 +00:00Commented Apr 2, 2014 at 19:24
-
\$\begingroup\$ Making it OO is a different ball of wax, I am afraid we don't re-write code, we just review it and sometimes propose an alternative way. \$\endgroup\$konijn– konijn2014年04月02日 19:37:52 +00:00Commented Apr 2, 2014 at 19:37
-
\$\begingroup\$ is there another stackexchange outlet I should be asking this in? Im really just trying to gain the basic concept of this OOJS stuff and would love it if I could see how someone with advanced jquery/js skills would approach the module I have included \$\endgroup\$NicholasAbrams– NicholasAbrams2014年04月02日 19:58:08 +00:00Commented Apr 2, 2014 at 19:58