$("#browsable .items:not(img.active)").hover(
function() { $(this).attr("src", $(this).attr("src").match(/[^\.]+/) + "_hover.png"); },
function() { $(this).attr("src", $(this).attr("src").replace("_hover", "")); }
);
$("#browsable .items img").click(
function() {
if ($(this).hasClass("active")) { return; }
var src = $(this).attr("src");
$("#browsable .items img").removeClass("active")
$("#browsable .items img").each(function(id, item) {
$(item).attr("src", $(item).attr("src").replace("_active", ""));
});
$(this).addClass("active");
$(this).attr("src", src.match(/[^\.]+/) + "_active.png");
}
);
Refactorings
No refactoring yet !
lomax
September 24, 2010, September 24, 2010 17:25, permalink
I would consider this an opportunity to encapsulate the logic in an object class that tracks the state you care about. I've preserved your logic, just reorganized the construction a little.
I also think that setting the classes is a pretty good idea, as it gives you more explicit control in the css, so I added class management to the hover/unhover event handlers. In fact, I wonder if you could simply set the images that you have to work with to simply load a transparent png? Then you could handle the image change in the classes in the css. That would satisfy both the stated image requirement (which sounds like it could be due to backwards compatibility or accessibility?), remove the need for regular expressions (and the src attribution), and let you set the background-images in css. If so, you can just remove the .attr("src" lines. If not, remove (or leave) the .addClass/.removeClass lines, they're no longer necessary to the code.
P.S. Since there was no sample page, this is untested.
function ImageEffect(){
var _activeImage;
var _selector;
//ctor
function ImageEffect(selector){
_activeImage=null;
_selector=selector;
bindEvents();
}
ImageEffect.apply(this,arguments);
//public methods
this.dispose=function(){
_activeImage=_selector=null;
}
//private methods
function bindEvents(){
var elements=$(_selector);
elements.hover(Image_MouseOver,Image_MouseOut);
elements.click(Image_Click);
}
//events
function Image_MouseOver(e){
var image=$(this);
if(image==_activeImage)return;
image.addClass("hover");
image.attr("src", image.attr("src").match(/[^\.]+/) + "_hover.png");
}
function Image_MouseOut(e){
var image=$(this);
image.removeClass("hover");
image.attr("src", image.attr("src").replace("_hover", ""));
}
function Image_Click(e){
var image=$(this);
if(image==_activeImage)return;
if(_activeImage){
_activeImage.removeClass("active");
_activeImage.attr("src", _activeImage.attr("src").replace("_active", ""));
}
image.addClass("active");
image.attr("src", image.attr("src").match(/[^\.]+/) + "_active.png");
_activeImage=image;
}
}
// inside page load function
var imageHandler=new ImageEffect("#browsable .items img");
// inside unload function
imageHandler.dispose();
I have this jQuery code I wrote the other day that (I'm fairly unhappy with) for managing the different states of an image, the image itself (image.png), the hover state (image_hover.png), and the active state for when it's clicked on (image_active.png).
I think I might be ok with the "hover" code, but the "active" code just looks like it could be done in a much better way. The code references an active class, which is only so I can use jQuery to differentiate between activated images and non-activated images (this is because there can only be one activated image at a time). I feel like there is a probably a way to do this where that class is unnecessary.
Anyways, I'd appreciate any suggestions! And by the way, before you suggest it, I would of course normally do this in CSS, but the way that the markup is coded, it's a requirement that I have to actually use an img tag in this instance (I can't use a sprite with all the images contained and just shift it around using background-position, as easy as that would make all of this!)