2
\$\begingroup\$

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>
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

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.
\$\endgroup\$
2
  • 1
    \$\begingroup\$ 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?) \$\endgroup\$ Commented Nov 8, 2012 at 13:35
  • \$\begingroup\$ 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. \$\endgroup\$
    – jsanc623
    Commented Nov 8, 2012 at 18:22

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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