Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Okay, so I wrote this function yesterday for one of our websites here at the office, the site is responsive, but one of the banners wasn't behaving at a certain screen size (was getting chopped off when screen size was less than 1067px). Basically, this script checks for window size of 1067px (higher than 1068px, banner is fine) to 980px (lower than 981, banner is hidden)

function updateReadyGoBG(){ 
    var vpWidth, difference, position = 0;
    vpWidth = jQuery(this).width();

    if(vpWidth > 980 && vpWidth < 1067){
        position = jQuery(".ready_go_bg").css("left").replace("px", "")
        difference = (position - (600 - (vpWidth - position) ) ) + 100;
        jQuery(".ready_go_bg").css("left", difference + "px")
    }
}

Any ideas on how to improve this to improve performance or reduce calls? Any review, refactoring or just general thoughts would be highly appreciated. What did I do wrong? What would you change?

Thanks all!

EDIT: adding rudimentary html snipped this acts on

<style>
.banner-container{ width:1000px; height:150px; }
.banner{ width: 100% }
.ready_go_bg{ position:absolute; left:600px; top:100px; }
</style>

<div class="banner-container">
    <div class="banner">
        <span class="ready_go_bg">Ready. Go.</span>
    </div>
</div>
share|improve this question
add comment

1 Answer

up vote 3 down vote accepted

Ok, assuming:

  • You didn't use media queries for cross-browser reasons
  • This function is already bound to a resize event, since you didn't include what triggers the function call
  • Assuming that this is the window object, judging from the way you call it vp as in viewport and getting its width.

Then:

function updateReadyGoBG(){ 

        //you can actually merge declaration and assignment
    var vpWidth   = jQuery(this).width(),
        //cache the ready_go_bg jQuery object to avoid requerying
        readyGoBg =  jQuery('.ready_go_bg'),
        position  = 0,
        difference;

    if(vpWidth > 980 && vpWidth < 1067){

        //you could use position instead of getting the CSS left
        //to avoid getting a string, and chopping off the "px" part
        position = readyGoBg.position().left;

        difference = (position - (600 - (vpWidth - position) ) ) + 100;

        //sadly, position is just a "getter".
        //you could use .offset(value) but calculating relative
        //to your offset parent would be too much work
        //we use .css(prop,value) to set the left value
        //if unit wasn't specified, jQuery assumes it's in pixels
        readyGoBg.css('left', difference);
    }
}

function updateReadyGoBG() {
    var vpWidth = jQuery(this).width(),
        readyGoBg = jQuery('.ready_go_bg'),
        position = 0,
        difference;
    if (vpWidth > 980 && vpWidth < 1067) {
        position = readyGoBg.position().left;
        difference = (position - (600 - (vpWidth - position))) + 100;
        readyGoBg.css('left', difference);
    }
}

General Advice:

  • Never forget ; at every line. You may run into issues during minification if you forget them
  • Though personal preference, but it's cleaner to use ' instead of ".
  • Unless totally necessary, I'd avoid absolute positioning. Let the layout dictate positions.
share|improve this answer
1  
1. Use window instead of this, because it's confusing where this points to without the context where the function is bound. 2. readyGoBg.css('left' add 'px' to the end. Otherwise it's not clear what you're setting (percentages, unicorns?) –  Jan Jongboom Nov 8 '12 at 13:35
    
Most of your assumptions are correct. Media queries are being used, but the banner was breaking on active resize of the browser window. This function is indeed already bound to a jQuery(window).resize() event - thus the this instead of window to save a call. Thank you for this! More than I expected. –  jsanc623 Nov 8 '12 at 18:22
add comment

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.