Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I am working on a simple mouseout and mouseover functions. Basically, I have color boxes (or swatches) and I have to get the color of the specific box when hovered and when selected. You can see how it works in the fiddle.

So I came out with a working script. But I am not sure how I can optimize or shorten the script since I will be having several color swatches. I made 2 samples swatches in this demo.

The question: is there a way to optimize/shorten it? Contents in the script were almost the same for each functions. The class names were the only ones changed.

/* COLOR SWATCH 1*/

$(".color").mouseover(function() {
   var hex = $( this ).css("background-color");
   var hexVal = $( this ).attr("value");
   var styles = {
      backgroundColor : hex
    };
    $('.selected-color').css(styles);
    $('.selected-color').text(hexVal);
});

$(".color").mouseout(function() {
    var hex = $('.active').css( "background-color");
    var hexValue = $('.active').attr("value");
    var styles = {
      backgroundColor : hex
    };
    $('.selected-color').empty();
    $('.selected-color').css(styles);
    $('.selected-color').text(hexValue);
});


$('.color').on('click', function() {
   $(this).addClass('active').siblings().removeClass('active');
    var hex = $(this).css("background-color");
    var hexVal = $('.active').attr("value");
    var styles = {
      backgroundColor : hex
    };
    $('.selected-color').css(styles);
    $('.selected-color').text(hexVal);
}); 


/* COLOR SWATCH 2*/

$(".color-2").mouseover(function() {
   var hexs = $( this ).css("background-color");
   var hexVal = $( this ).attr("value");
   var styles = {
      backgroundColor : hexs
    };
    $('.selected-color-2').css(styles);
    $('.selected-color-2').text(hexVal);
});

$(".color-2").mouseout(function() {
    var hex = $('.active-2').css( "background-color");
    var hexValue = $('.active-2').attr("value");
    var styles = {
      backgroundColor : hex
    };
    $('.selected-color-2').empty();
    $('.selected-color-2').css(styles);
    $('.selected-color-2').text(hexValue);
});


$('.color-2').on('click', function() {
   $(this).addClass('active-2').siblings().removeClass('active-2');
    var hex = $(this).css("background-color");
    var hexVal = $('.active-2').attr("value");
    var styles = {
      backgroundColor : hex
    };
    $('.selected-color-2').css(styles);
    $('.selected-color-2').text(hexVal);
}); 
.color-swatch div {
    width: 25px;
    height: 25px;
    float: left;
    border: 1px solid #313131;
    margin-right: 5px;
  }

.selected-color, .selected-color-2 {
    width: 90px;
    height: 20px;
    color: #FFF;
    text-align: center;
    padding: 2px;

}  
.active, .active-2 {
    border: 3px solid #151515 !important;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.0.0/jquery.min.js"></script>

<!-- COLOR SWATCH 1 -->
<p>SELECTED COLOR 1: <span class="selected-color" style="background-color:#ef4060;" value="Pink">Pink</span></p> 
<div class="color-swatch">
     <div class="color" style="background-color:#028482;" value="Aqua"></div>
     <div class="color" style="background-color:#4c0055;" value="Purple"></div>
     <div class="color active" style="background-color:#ef4060;" value="Pink"></div>         
</div>

<!-- COLOR SWATCH 2 -->
<br><br>
<p>SELECTED COLOR 2: <span class="selected-color-2" style="background-color:#028482;" value="Aqua">Aqua</span>
</p> 
<div class="color-swatch">
     <div class="color-2 active-2" style="background-color:#028482;" value="Aqua"></div>
     <div class="color-2" style="background-color:#4c0055;" value="Purple"></div>
     <div class="color-2" style="background-color:#ef4060;" value="Pink"></div>         
</div>

share|improve this question
up vote 6 down vote accepted

HTML abuse

The HTML5 specification does not allow a <div> element to have a value attribute. You can use a data-* attribute (named data-color, for example). A title attribute would also be reasonable.

You are misusing CSS classes as IDs. An ID is a unique name for an individual element. A class is a set of elements that should all be treated alike. You should not have color and color-2 as two class names. Rather, they should be a single class.

Confused terminology

Your wording is a bit off, in my opinion. Each individual sample is a "swatch". A color selector is a widget consisting of three swatches.

When you make a decision by clicking on a swatch, the swatch isn't just "active" — I would say that that color has been selected.

Based on the above, I would rename…

  • the color and color-2 classes both to swatch
  • the color-swatch class to swatch-selector
  • the active class to selected

jQuery

Once you rename the classes so that both widgets have the same class names, it becomes possible to write code that treats the swatch selector as a generic widget.

If you need to define both a mouseover and a mouseout handler, use hover() instead.

$(function() {
    'use strict';

    function showActiveColor($selector) {
        var selectorId = $selector.attr('id');
        var $selected = $selector.find('.swatch.selected');
        $('#active-' + selectorId).empty()
                                  .text($selected.data('color'))
                                  .css({ backgroundColor: $selected.css('background-color') });
    }

    $('.swatch-selector .swatch').hover(
        function mouseOver(event) {
            var colorName = $(this).data('color');
            var color = $(this).css('background-color');
            var selectorId = $(this).closest('.swatch-selector').attr('id');
            $('#active-' + selectorId).text(colorName)
                                      .css({ backgroundColor: color });
        },
        function mouseOut(event) {
            showActiveColor($(this).closest('.swatch-selector'));
        }
    ).click(function onClick(event) {
        $(this).addClass('selected')
               .siblings().removeClass('selected');
    });

    // Initialization so that you don't have to hard-code the span's
    // background-color and text to match the initial selection
    $('.swatch-selector').each(function(index, el) { showActiveColor($(el)); });
});
.swatch-selector .swatch {
    width: 25px;
    height: 25px;
    float: left;
    border: 1px solid #313131;
    margin-right: 5px;
}
.swatch-selector .swatch.selected {
    border: 3px solid #151515;
}
.color-name {
    width: 90px;
    height: 20px;
    color: #FFF;
    text-align: center;
    padding: 2px;
}  
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.0.0/jquery.min.js"></script>

<!-- COLOR SWATCH SELECTOR 1 -->
<p>SELECTED COLOR 1: <span class="color-name" id="active-color-1"></span></p> 
<div class="swatch-selector" id="color-1">
     <div class="swatch" style="background-color:#028482;" data-color="Aqua"></div>
     <div class="swatch" style="background-color:#4c0055;" data-color="Purple"></div>
     <div class="swatch selected" style="background-color:#ef4060;" data-color="Pink"></div>         
</div>

<br><br>

<!-- COLOR SWATCH SELECTOR 2 -->
<p>SELECTED COLOR 2: <span class="color-name" id="active-color-2"></span></p> 
<div class="swatch-selector" id="color-2">
     <div class="swatch selected" style="background-color:#028482;" data-color="Aqua"></div>
     <div class="swatch" style="background-color:#4c0055;" data-color="Purple"></div>
     <div class="swatch" style="background-color:#ef4060;" data-color="Pink"></div>         
</div>

share|improve this answer
    
This is a very good answer, but wouldn't it be better to declare the variables outside the hover function to prevent re-creating them every time the event fires? – Ahmed Jan 29 at 20:20

I thought store the result of $('.selected-color') will be better since that DOM element is always there. Reduce jQuery selector will be good for performance.

share|improve this answer

Yes, you can just make a function that takes the class names as parameters like this:

function attachMouseEvents(elementClass, selectedColorClass, activeClass) {
    $(elementClass).mouseover(function() {
       var hex = $( this ).css("background-color");
       var hexVal = $( this ).attr("value");
       var styles = {
          backgroundColor : hex
        };
        $(selectedColorClass).css(styles);
        $(selectedColorClass).text(hexVal);
    });

    // apply same principle for mouseover and mouseout ...
}

attachMouseEvents('.color', '.selected-color', '.active');
attachMouseEvents('.color-2', '.selected-color-2', '.active-2');
share|improve this answer
    
Hi @lex82. Thank you, it worked when I added the mouseout event with the same layout. But it didn't run properly on my .click function. Did I miss out something? Updated fiddle: jsfiddle.net/b0o9xhr0/3 – novice Jan 29 at 9:34
    
You forgot to replace two occurrences of '.selected-color-2' with selectedColorClass. – lex82 Jan 29 at 10:41

By containing the individual colour selection blocks in a parent element with an appropriate class name, you can write you functions to find the correct siblings:

<!-- COLOR SWATCH 1 -->
<div class="swatch-holder">
    <p>SELECTED COLOR 1: <span class="selected-color" style="background-color:#ef4060;" value="Pink">Pink</span></p> 
  <div class="color-swatch">
     <div class="color" style="background-color:#028482;" value="Aqua"></div>
     <div class="color" style="background-color:#4c0055;" value="Purple"></div>
     <div class="color active" style="background-color:#ef4060;" value="Pink"></div>         
  </div>
</div>

<!-- COLOR SWATCH 2 -->
<div class="swatch-holder">

  <p>SELECTED COLOR 2: <span class="selected-color" style="background-color:#028482;" value="Aqua">Aqua</span>
  </p> 
  <div class="color-swatch">
     <div class="color active" style="background-color:#028482;" value="Aqua"></div>
     <div class="color" style="background-color:#4c0055;" value="Purple"></div>
     <div class="color" style="background-color:#ef4060;" value="Pink"></div>         
  </div>
</div>

JS:

$(".color").mouseover(function() {
    var holder = $(this).parents('.swatch-holder');
    var selectedColor = holder.find('span.selected-color');
    var hex = $( this ).css("background-color");
    var hexVal = $( this ).attr("value");
    var styles = {
        backgroundColor : hex
    };
    //jquery methods are chainable:  
    selectedColor.css(styles).text(hexVal);

});

$(".color").mouseout(function() {
    var holder = $(this).parents('.swatch-holder');
    var selectedColor = holder.find('span.selected-color');
    var hex = holder.find('.active').css( "background-color");
    var hexVal = holder.find('.active').attr("value");
    var styles = {
      backgroundColor : hex
    };  
    selectedColor.css(styles).text(hexVal);
});


$('.color').on('click', function() {
    $(this).addClass('active').siblings().removeClass('active');
    var holder = $(this).parents('.swatch-holder');
    var selectedColor = holder.find('span.selected-color');
    var hex = $(this).css("background-color");
    var hexVal = $(this).attr("value");
    var styles = {
      backgroundColor : hex
    };  
    selectedColor.css(styles).text(hexVal);
}); 

Note there is still an amount of duplication in the 3 functions themselves, and as pointed out in another answer, you should use data- attributes rather than value on your div.color elements, but this is a start.

Fiddle: https://jsfiddle.net/mwx2pr6k/1/

Note i also edited the css a little

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.