2
\$\begingroup\$

I have changed a lot from my original code that I posted about a week ago, you can see that here:

Guild Wars: Knights & Dragons

Now what I need to know is if I improve this code further. Any help is greatly appreciated.

EDIT: Should I add <title> tags to my page to make it more accessible? And also <meta> keywords? I have never used it so I'm not entirely sure how it works and why to use them.

HTML:

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title></title> 
<link rel="stylesheet" href="pagestyling.css">
</head>

<body>

    <div id="menu">
        <div id="img"></div>
            <div>Gallery</div>
            <div>Stats</div>
            <div>Members</div>
            <div>GroupMe</div>
            <div id="knd">K&amp;D</div>
            <div>Apply</div>
    </div>

        <div id="guildinfo">
                <span class="info">Back to wars >></span>
        </div>

    <div id="popupwarsdiv"><h3>Knights & Dragons - Wars</h3>

            <ul>
                <li>1/7 80th place</li>
                <li>1/7 80th place</li>
                <li>1/7 80th place</li>
                        <li>1/7 80th place</li>
                        <li>1/7 80th place</li>
                        <li>1/7 80th place</li>
                                <li>1/7 80th place</li>
                                <li>1/7 80th place</li>
                                <li>1/7 80th place</li>
            </ul>

    <span class="info">Guild info >></span>
        <span id="previous">
            <a href="previouswars.html">Click here to see all previous wars</a>
        </span>
    </div> <!--End of popupwarsdiv-->

    <div id="content"></div> <!-- end of content -->


    <div id="content2"></div> <!-- end of content2 -->

<script src="http://ajax.googleapis.com/ajax/libs/jquery/1.10.2/jquery.min.js"></script>
<script src="javascript.js" type="text/javascript"></script>
</body>
</html>

CSS:

body { 
    margin: 0; 
    background: url(img/background.jpg) no-repeat fixed;
    background-size: cover;
    font-family: "Tahoma", "Arial", "Serif";
    font-size: 14px;
    color: white;
    text-align: center;
}

html, body {
    height: 100%;
}

a { 
    text-decoration: none;
    color: yellow;
}   

h1, h2, h3, h4, h5, h6 { 
    margin: 0; 
    color: yellow;
}

#menu {
    width: 316px;
    height: 100px;
    margin-top: 20px;
    margin-left: 50px;
}

#img { 
    background: url(img/icon.png); 
    background-size: cover; 
    width: 100px;
    height: 100px;
    float: left;
} 

.menuitems { 
    width: 70px;
    height: 30px;
    color: yellow;
    background: url(img/brown.jpg);
    font-family: "Arial";
    float: right;
    margin: 20px 2px -18px 0px;
    line-height: 30px;
    border-radius: 5px;
}
    .menuitems:hover {
        cursor: pointer;
    }

#popupwarsdiv { 
    width: 450px;
    height: 170px;
    border: 1px solid black;
    border-radius: 10px;
    background: url(img/brown.jpg);
    top: 20px;
    left: 380px;
    position: absolute;
    line-height: 30px;
    opacity: 0;
}

    #popupwarsdiv ul {
        list-style: none;
        -moz-column-count: 3;
        -webkit-column-count: 3;
        column-count: 3;
        float: left;
    }

    #popupwarsdiv li { 
        width: 110px;
        height: 30px;
        margin: 0;
    }

.info {
    position: absolute;
    top: 0px;
    right: 15px;
    font-size: 11px;
    color: white;       
}
    .info:hover {
        cursor: pointer;
    }
#previous { 
    position: absolute;
    bottom: 0px; 
    right: 15px;
    font-size: 11px;
}

#guildinfo {
    width: 450px;
    height: 170px;
    border: 1px solid black;
    border-radius: 10px;
    background: url(img/brown.jpg);
    top: 20px;
    left: 380px;
    position: absolute;
    opacity: 0;
    z-index: -1;
    line-height: 30px;
}

#content {   /* main content*/
    width: 700px;
    padding: 50px;
    min-height: 100%;
    background: rgba(0, 0, 0, 0.7);
    margin: 100px auto auto 50px;
}

#content2 { /*secondary content (right)*/
    width: 380px;
    min-height: 80%;
    background: rgba(0, 0, 0, 0.7);
    top: 380px;
    right: 80px;
    position: absolute;
    border-radius: 15px;
}

JavaScript:

$(document).ready(function() {

    $("#menu").children("div").slice(1,7).addClass("menuitems");

//THE FOLLOWING INFO FUNCTION NEEDS AN INTERNET EXPLORER FIX    

var guildInfo = $("#guildinfo");

$(".info").click(function() {
    if($(guildInfo).css('z-index') === '-1'){
        $(guildInfo).css('z-index', '1');
            $(guildInfo).animate({
            opacity:'1'
            }); //end of animate
}
    else{
        $(guildInfo).animate({
            opacity:'0'
        });

        setTimeout(function() {
            $(guildInfo).css('z-index', '-1');
        }, 500);

    }
}); //end of info function


$("#knd").click(function() {
    if($("#popupwarsdiv").css('opacity') === '0') {         
        $("#popupwarsdiv").animate({
        opacity:'1'
        }); //end of animate
    }

else {
    $("#popupwarsdiv").animate({
        opacity:'0'
    });  //end of animate
}

}); //end of KND click function

}); //end of doc
\$\endgroup\$
0
2
\$\begingroup\$

From a once over:

  • This:

    $("#menu").children("div").slice(1,7).addClass("menuitems");
    

    should have been set in the HTML, I see no good reason to do this with JavaScript. If you insist on doing it in JavaScript, have 1 and 7 be constants:

    $("#menu").children("div").slice(FIRST_MENU_ITEM,LAST_MENU_ITEM).addClass("menuitems");
    
  • Consider using strict mode.
  • Consider using http://jsbeautifier.org/, it makes your code more readable, especially the
  • Your code is a bit repetitive in places, for example here:

    $("#knd").click(function() {
        if($("#popupwarsdiv").css('opacity') === '0') {         
            $("#popupwarsdiv").animate({
            opacity:'1'
            }); //end of animate
        }
    
    else {
        $("#popupwarsdiv").animate({
            opacity:'0'
        });  //end of animate
    }
    

    You are basically calling .animate() almost exactly the same, you should find and extract the difference, determine the difference up front and then have one call. For example:

    var $popupwars = $("#popupwarsdiv"); //Cache this div reference
    $("#knd").click(function() {
        var oldOpacity = $popupwars.css('opacity'),
            newOpacity = oldOpacity === '0' ? '1' : '0';
        $("#popupwarsdiv").animate({ opacity:newOpacity }); 
    }
    

You could apply the same in $(".info").click(function() {

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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