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

This set of code is very large, and I'm hoping if someone could show me a more efficient way of doing this. In particular, the JavaScript code, but HTML and CSS corrections are just as welcome.

$(document).ready(function () {
				var item1 = "active";
				var item2 = "unactive";
				var item3 = "unactive";
				var item4 = "unactive";
				var item5 = "unactive";
				$("#ITEM1").click(function () {
					if (item2 = "active") {
						$("#item1").fadeIn(1500);
						$("#item2").fadeOut(0);
						item2 = "unactive";
						item1 = "active";
					}
					
					if (item3 = "active") {
						$("#item1").fadeIn(1500);
						$("#item3").fadeOut(00);
						item1 = "active";
						item3 = "unactive";
					}
					
					if (item4 = "active") {
						$("#item1").fadeIn(1500);
						$("#item4").fadeOut(00);
						item1 = "active";
						item4 = "unactive";
					}
					
					if (item5 = "active") {
						$("#item1").fadeIn(1500);
						$("item5").fadeOut(00);
					}
					
				});
				$("#ITEM2").click(function () {
					if (item1 = "active") {
						$("#item2").fadeIn(1500);
						$("#item1").fadeOut(0);
					}
					
					if (item3 = "active") {
						$("#item2").fadeIn(1500);
						$("#item3").fadeOut(0);
					}
					
					if (item4 = "active") {
						$("#item2").fadeIn(1500);
						$("#item4").fadeOut(0);
					}
					
					if (item5 = "active") {
						$("#item2").fadeIn(1500);
						$("#item5").fadeOut(0);
					}
				});
				
				$("#ITEM3").click(function () {
					if (item1 = "active") {
						$("#item3").fadeIn(1500);
						$("#item1").fadeOut(00);
					}
					
					if (item2 = "active") {
						$("#item3").fadeIn(1500);
						$("#item2").fadeOut(00);
					}
					
					if (item4 = "active") {
						$("#item3").fadeIn(1500);
						$("#item4").fadeOut(00);
					}
					
					if (item5 = "active") {
						$("#item3").fadeIn(1500);
						$("#item5").fadeOut(00);
					}
				});
				
				$("#ITEM4").click(function () {
					if (item1 = "active") {
						$("#item4").fadeIn(1500);
						$("#item1").fadeOut(00);
					}
					
					if (item2 = "active") {
						$("#item4").fadeIn(1500);
						$("#item2").fadeOut(00);
					}
					
					if (item3 = "active") {
						$("#item4").fadeIn(1500);
						$("#item3").fadeOut(00);
					}
					
					if (item5 = "active") {
						$("#item4").fadeIn(1500);
						$("#item5").fadeOut(00);
					}
				});
				
				$("#ITEM5").click(function () {
					if (item1 = "active") {
						$("#item5").fadeIn(1500);
						$("#item1").fadeOut(00);
					}
					
					if (item2 = "active") {
						$("#item5").fadeIn(1500);
						$("#item2").fadeOut(0);
					}
					
					if (item3 = "active") {
						$("#item5").fadeIn(1500);
						$("#item3").fadeOut(0);
					}
					
					if (item4 = "active") {
						$("#item5").fadeIn(1500);
						$("#item4").fadeOut(0);
					}
				});
			});
#menuItems {
				background-color: #e6e6e6;
			}
			
			#itemLayer {
			padding-top: 25px;
			padding-bottom: 800px;
			}
			
			#menuNav h2 {
			font-family: 'Arvo';
			
			text-align: center;
			}
			
			#menuNav ul {
			float: left;
			
			background-color: white;
			}
			
			#menuNav li {
			padding: 20px;
			margin-top: 5px;
			font-size: 1.3em;
			font-family: 'Lato';
			font-weight: 300;
			list-style-type: none;
			cursor: pointer;
			transition: .5s;
			}
			
			#menuNav li:hover {
				background-color: gray;
				color: white;
			}
			
			#item2, #item3, #item4, #item5 {
			display: none;
			}
			
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<section id="menuItems">
			<div id="itemLayer">
			<div id="menuNav">
				<h2>Menu Items</h2>
				<ul>
					<li id="ITEM1"><span class="borderText">Item 1</span></li>
					<li id="ITEM2"><span class="borderText">Item 2</span></li>
					<li id="ITEM3"><span class="borderText">Item 3</span></li>
					<li id="ITEM4"><span class="borderText">Item 4</span></li>
					<li id="ITEM5"><span class="borderText">Item 5</span></li>
				</ul>
			</div>
			
			<div id="itemDisplay">
				<div class="items" id="item1">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item1" />
					<h3>Item 1</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item2">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item2" />
					<h3>Item 2</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item3">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item3" />
					<h3>Item 3</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item4">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item4" />
					<h3>Item 4</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item5">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item5" />
					<h3>Item 5</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
			</div>
			</div>
		</section>

share|improve this question
1  
Am I the only one to have notice that you use = instead of == for comparisons? Each of your ifs will always be executed... – jcaron Jan 27 at 8:46
    
Thank you jcaron – HTMLNoob Jan 27 at 9:27
up vote 2 down vote accepted

Copy-and-paste bug

You have a bug due to a careless omission of a # character:

if (item5 = "active") {
    $("#item1").fadeIn(1500);
    $("item5").fadeOut(00);
}

Since there is no <item5> element, if you click on Item 5, then Item 1, you'll get both items simultaneously displayed.

Semantic markup and graceful degradation

The first principle of web design with JavaScript is that the page should work as well as possible even without any JavaScript. The best way to make that happen would be to change your navigation items from

<li id="ITEM1"><span class="borderText">Item 1</span></li>

… into semantically significant <a> tags:

<li><a href="#item1" class="borderText">Item 1</a></li>

You'll probably want to style those <a> elements as if they were a plain old <span>.

I've also gotten rid of the confusing id="ITEM1" and id="item1" distinction.

In addition, you need to get rid of this CSS rule:

#item2, #item3, #item4, #item5 {
  display: none;
}

… and implement it using JavaScript instead. That way, if JavaScript is disabled, items 2 to 5 will still be visible.

Since you've used the HTML5 <section> element, you'll also want to use <nav> for the navigation links.

Strategy

The solution is simple: instead of keeping track of five active/inactive states, just keep track of which one is currently active. When a menu item is clicked, fade out the active item (unless it happens to be the one that was just clicked), and fade in the newly selected item.

"use strict";

$(document).ready(function () {
    var $active = $('#item1');
    $('.items').not($active).hide();

    function activate($item) {
        $item.fadeIn(1500);
        $active.not($item).fadeOut(0);
        $active = $item;
    }

    $('#menuNav li').click(function(event) {
        event.preventDefault();
        activate($($(this).find('a').attr('href')));
    });
});
#menuItems {
   background-color: #e6e6e6;
}

#itemLayer {
    padding-top: 25px;
    padding-bottom: 800px;
}

#menuNav h2 {
    font-family: 'Arvo';
    text-align: center;
}

#menuNav ul {
    float: left;
    background-color: white;
}

#menuNav li {
    padding: 20px;
    margin-top: 5px;
    font-size: 1.3em;
    font-family: 'Lato';
    font-weight: 300;
    list-style-type: none;
    cursor: pointer;
    transition: .5s;
}

#menuNav li:hover {
    background-color: gray;
    color: white;
}

/* Make menuNav links look like regular text */
#menuNav li > a {
    text-decoration: none;
    color: inherit;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<section id="menuItems">
    <div id="itemLayer">
        <div id="menuNav">
            <h2>Menu Items</h2>
            <ul>
                <li><a href="#item1" class="borderText">Item 1</a></li>
                <li><a href="#item2" class="borderText">Item 2</a></li>
                <li><a href="#item3" class="borderText">Item 3</a></li>
                <li><a href="#item4" class="borderText">Item 4</a></li>
                <li><a href="#item5" class="borderText">Item 5</a></li>
            </ul>
        </div>
        
        <div id="itemDisplay">
            <div class="items" id="item1">
                <img src="food-green.jpeg" width="450px" height="250px" alt="item1" />
                <h3>Item 1</h3>
                <p>Lorem ipsum sit amet</p>
            </div>
            
            <div class="items" id="item2">
                <img src="food-green.jpeg" width="450px" height="250px" alt="item2" />
                <h3>Item 2</h3>
                <p>Lorem ipsum sit amet</p>
            </div>
            
            <div class="items" id="item3">
                <img src="food-green.jpeg" width="450px" height="250px" alt="item3" />
                <h3>Item 3</h3>
                <p>Lorem ipsum sit amet</p>
            </div>
            
            <div class="items" id="item4">
                <img src="food-green.jpeg" width="450px" height="250px" alt="item4" />
                <h3>Item 4</h3>
                <p>Lorem ipsum sit amet</p>
            </div>
            
            <div class="items" id="item5">
                <img src="food-green.jpeg" width="450px" height="250px" alt="item5" />
                <h3>Item 5</h3>
                <p>Lorem ipsum sit amet</p>
            </div>
        </div>
    </div>
</section>

share|improve this answer

Came up with something like this.
Added #buttons id on buttons list and .active class on currently active element in HTML part.
I hope it will be of some help.

$(document).ready(function () {

    $('#buttons li').click(function () {
        var activeEl = $('.items.active')[0];
        var $newActiveEl = $('#'+this.id.toLowerCase());

        $(activeEl).fadeOut(0);
        $(activeEl).removeClass('active');

        $newActiveEl.fadeIn(1500);
        $newActiveEl.addClass('active');
    });
});
#menuItems {
				background-color: #e6e6e6;
			}
			
			#itemLayer {
			padding-top: 25px;
			padding-bottom: 800px;
			}
			
			#menuNav h2 {
			font-family: 'Arvo';
			
			text-align: center;
			}
			
			#menuNav ul {
			float: left;
			
			background-color: white;
			}
			
			#menuNav li {
			padding: 20px;
			margin-top: 5px;
			font-size: 1.3em;
			font-family: 'Lato';
			font-weight: 300;
			list-style-type: none;
			cursor: pointer;
			transition: .5s;
			}
			
			#menuNav li:hover {
				background-color: gray;
				color: white;
			}
			
			#item2, #item3, #item4, #item5 {
			display: none;
			}
			
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<section id="menuItems">
			<div id="itemLayer">
			<div id="menuNav">
				<h2>Menu Items</h2>
				<ul id="buttons">
					<li id="ITEM1"><span class="borderText">Item 1</span></li>
					<li id="ITEM2"><span class="borderText">Item 2</span></li>
					<li id="ITEM3"><span class="borderText">Item 3</span></li>
					<li id="ITEM4"><span class="borderText">Item 4</span></li>
					<li id="ITEM5"><span class="borderText">Item 5</span></li>
				</ul>
			</div>
			
			<div id="itemDisplay">
				<div class="items active" id="item1">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item1" />
					<h3>Item 1</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item2">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item2" />
					<h3>Item 2</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item3">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item3" />
					<h3>Item 3</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item4">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item4" />
					<h3>Item 4</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
				
				<div class="items" id="item5">
					<img src="food-green.jpeg" width="450px" height="250px" alt="item5" />
					<h3>Item 5</h3>
					<p>Lorem ipsum sit amet</p>
				</div>
			</div>
			</div>
		</section>

share|improve this answer
    
Nice first answer. Welcome to Code Review! – 200_success Jan 27 at 8:14
    
Thank you kkopec I +1 your answer. – HTMLNoob Jan 27 at 8:32

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.