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.

I've been developing a page that is designed like this:

______________
|  |         |
|  |         |
|  |         |
--------------

Everything is wrapped in a wrapper, then there are 2 divs inside it, menu and content. They're both floated to the left so that way I don't have to fiddle with CSS and give myself a headache. Then, to ensure that content always spans the entire page, even upon resize:

var contentWidth;
var allPenta = '#penta1,#penta2,#penta3,#penta4';
var allButton = '.button1,.button2,.button3,.button4';
var allFullPenta = '#pentafull1,#pentafull2,#pentafull3,#pentafull4'
function simplyWidth(wrapper, menu1, content, pentas){
    var menu = $(menu1).width();
    var wrapperwidth = $(wrapper).width();
    var newWidth = wrapperwidth-menu;
    var widgetWidth = newWidth/4;
    contentWidth = newWidth;

    $(content).css("width", newWidth);
    $(pentas).css("width", widgetWidth);
};

Inside content, I also have 4 more divs which will serve as "tiles" that you can hover over. That is what "pentas" is. In the code above, I also calculate the width of each penta (4 of them to be exact) and set their width to equal what content is.

Then I wanted to make sure that the tiles weren't under or above the footer div, so I developed another function to calculate the height dynamically even on resize:

var contentHeight
function simplyHeight(footer, bottom, wrapper, pentas){
    var footheight = $(footer).height();
    var bottomalign = $(bottom).height();
    var wrapperheight = $(wrapper).height();
    var newHeight = footheight + bottomalign;
    var newnewHeight = wrapperheight - newHeight;
    contentHeight = newnewHeight;

    $(pentas).css("height", newnewHeight);

};
function simplyDimension(){
    $(allFullPenta).css("width", contentWidth);
    $(allFullPenta).css("height", contentHeight);
}

After that, I wanted the user to hover over them, and for them to transition from nearly no opacity, to about ~50% opacity (in relation to my original CSS, despite the code changing it from .3 to 1):

var stoppable = true;
function simplyHighlight(penta){
    $(penta).fadeTo(0,.3)

    $(penta).mouseenter(function(){
        if (stoppable == true) {
            $(penta).stop();
            $(penta).fadeTo(200,1);
        };
    });
    $(penta).mouseleave(function(){
        if (stoppable == true) {
            $(penta).stop();    
            $(penta).fadeTo(300,.3);
        };
    });

}

This is the part I find "iffy":

function simplySpan(button, fullpenta){
    $(button).click(function(){
        stoppable = false;
        $(allPenta).hide(400);
        $(allFullPenta).hide(400);
        $(fullpenta).show(400);
    });
};

When ever the user goes over to one of the tiles, and clicks the button to "expand it", it basically hides the tiles and shows a hidden div which acts as the tile that "expanded" holding more information. I encountered a problem awhile back where if you clicked the button, it would hide the divs, but if you didn't move away, the previous function of "highlighting" would activate, stopping all animations and keep the div in place while the others disappeared. To fix this issue, I created a switch using an if statement where it turns off that ability to "change the opacity" when activating that button. It works just as I wanted it to, but I think I might have over-engineered my solution. I'm not exactly the most proficient in JavaScript and jQuery, so I think there is a solution to condense my fix.

Lastly is my "master function" which is the function I use to hold all my other functions functions (lol) to be called as a function in my HTML document so I don't have to have all this script in my way while code HTML:

function simplyMaster(){
    simplyHeight('#contentfoot', '.bottomalign', '#wrapper', '#penta1,#penta2,#penta3,#penta4');
    simplyWidth('#wrapper','#menu1','#content','#penta1,#penta2,#penta3,#penta4');
    $(window).resize(function(){
        simplyWidth('#wrapper','#menu1','#content','#penta1,#penta2,#penta3,#penta4');
        simplyHeight('#contentfoot', '.bottomalign', '#wrapper', '#penta1,#penta2,#penta3,#penta4');
        simplyDimension()
    });
    simplyHighlight('#penta1');
    simplyHighlight('#penta2');
    simplyHighlight('#penta3');
    simplyHighlight('#penta4');
    simplyDimension();
    $(allFullPenta).hide();
    simplySpan('.button1', '#pentafull1');
    simplySpan('.button2', '#pentafull2');
    simplySpan('.button3', '#pentafull3');
    simplySpan('.button4', '#pentafull4');
};

For all you naughty code gurus out there, what do you think of my code? How bad is it? This is by far the most jQuery I've ever coded which doesn't say much for all you try-hards out there, but I'd like to know and be shown what I can do to compress, condense, and simplify my code. Also, if there is a better way to remedy my issue above.

The website I'm doing this for is linked right here on the original website I'm redesigning:

The new Rockford Driveline Website

share|improve this question
2  
So instead of doing the resize in CSS via media queries, you're opting to do it with JavaScript? What if I have JS turned off? Seems to me that you're going about this the wrong (and overly complicated) way by doing it in JS rather than with CSS media queries. –  jsanc623 Jul 16 at 17:32
    
Just to clarify, you're using JavaScript to create Equal Height Columns? –  cimmanon Jul 16 at 17:32
    
The reason why I can't do this (or that I know of) is that when I attempted to do it in CSS is that any width larger than the view port removes the div from display. (Due to the use of Float.) So, to remedy this issue, I have made the code figure out the width of #content and then adjust the 4 tiles to be 1/4 of the content and set each of them that size. When the window is resize, it adjusts the width accordingly. I also do it for the height as well, because as I said, if it reaches outside the viewport, it disappears. That is why I did it in Javascript. –  Teknikitsune Jul 16 at 17:37
    
And yes, I'm using javascript to manage everything with the height and width (as far as the #content part is concerned). Also, if someone is using their browser with javascript turned off, then I share no pity. –  Teknikitsune Jul 16 at 17:40
    
We cannot review broken code (the updated code you've just added). That should be asked on Stack Overflow instead. –  Jamal Jul 16 at 19:45

2 Answers 2

up vote 1 down vote accepted

The code below approximates the current functionality of the site that you linked to, using only CSS percentages. You can target certain screen sizes (for example, this code below breaks at 830px) by utilizing media queries.

<html>
<head>
    <style>
        .wrapper{
            width: 100%;
            min-height: 900px;
            border: 1px solid black;
            display: block;
        }
        .sidebar{
            width: 30%;
            height: 100%;
            border: 1px dotted red;
            float: left;
        }
        .content{
            width: 69.5%;
            height: 100%;
            border: 1px dashed red;
            float: right;
        }
        .tile{
            width: 24%;
            height: 100%;
            border: 1px solid blue;
            display: inline-block;
            margin: 0;
        }
        .tile h1{

        }
        .tile p{

        }
    </style>
</head>
<body>
    <div class="wrapper">
        <div class="sidebar"></div>
        <div class="content">
            <div class="tile">
                <h1>Tile 1</h1>
                <p>1</p>
            </div>
            <div class="tile">
                <h1>Tile 2</h1>
                <p>2</p>
            </div>
            <div class="tile">
                <h1>Tile 3</h1>
                <p>3</p>
            </div>
            <div class="tile">
                <h1>Tile 4</h1>
                <p>4</p>
            </div>
        </div>

        <div style="clear:both"></div>
    </div>
</body>
</html>
share|improve this answer
    
This is actually what I was hoping someone would reply with. If I can mimic the same process I had before with minimal jquery, then I'll be really happy. Thank you for sharing your answer. I'll begin testing right away. –  Teknikitsune Jul 16 at 19:09
1  
This uses no jQuery at all :) and as I mentioned, doing media queries gives you far more control. Not to mention that CSS carries over more easily than javascript, and is faster. –  jsanc623 Jul 16 at 19:13
1  
Additionally - I would suggest you look perhaps into Bootstrap or Zurb Foundation as they take care of responsiveness for you. –  jsanc623 Jul 16 at 19:26
1  
For the page scrolling down - just modify the margins of the divs to 0 and adjust the percentages to be closer together once the margins and borders are gone. –  jsanc623 Jul 16 at 20:09
1  
Thanks for the help, I've got everything working with your help. The insight and references are going to help a lot. I had to add comment tags between each inline-block tile so that the area doesn't register it as a "space" as if I placed each div right next to each other. –  Teknikitsune Jul 16 at 20:12

There is a very good reason JavaScript is typically not used to control the layout of webpages: it is slow. Really slow. If you you have a choice between CSS and JavaScript to create a particular effect, then CSS is going to be faster, hands down, in just about every situation (If you find an instance where JS is faster, I really want to know).

If you want equal height columns, there are a multitude of ways to do so using pure CSS. My preferred method is using the table display properties. Unlike floats, this will guarantee that your elements will not wrap:

.parent {
    display: table;
    width: 100%;
}
.child {
    display: table-cell;
}

However, creating a design in this day and age that only works when your browser's viewport is greater than some arbitrary width is frowned on (some people like to claim that this was an acceptable practice several years ago, but I never shared that opinion). Your site will fail to appear modern because of it.

Your fade effect can also be done with pure CSS:

.foo {
    opacity: .5;
    transition: opacity 2s;
}
.foo:hover {
    opacity: 1;
}

I should also point out that the default faded state of your elements reminds me of mystery meat navigation. Content is the most important thing on your page, but you're hindering the user from being able to read it. Even when the elements are fully saturated, they are difficult to read over the background image.

share|improve this answer
    
Thank you, I didn't do that in the first place because I was on a JQuery tangent and decided that it'd be simpler to do it in JQuery. Anyway, as for as your opinion of my design, I see where you're coming from. It's still a work in progress, and I'm adapting as I go. My boss likes the direction I'm headed, so I'll just keep it at that. –  Teknikitsune Jul 16 at 19:39

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.