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>
2 Answers 2
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>
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>
-
\$\begingroup\$ Nice first answer. Welcome to Code Review! \$\endgroup\$200_success– 200_success2016年01月27日 08:14:26 +00:00Commented Jan 27, 2016 at 8:14
=
instead of==
for comparisons? Each of yourif
s will always be executed... \$\endgroup\$