2
\$\begingroup\$

I have this code currently written to help me create a menu. I was just wondering if there is a way to tidy this so that there is less code needed to achieve the same thing.

The code is designed to create large dropdowns which have divs inside which will contain most of the content. I want to hide parts of the menu so that the page doesn't become crowded so Its set to hide the other dropdown containers.

$(document).ready(function() {
 $("#1_open").click(function() {
 $("#background_container").slideToggle((500, 'easeInOutExpo'), function() {
 var background = $(this).css('display');
 if (background == 'none') {
 $('#1_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
 } else {
 $('#1_arrow').attr('src', './resources/images/icons/left.png');
 $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
 }
 });
 });
 $("#2_open").click(function() {
 $("#colour_container").slideToggle((500, 'easeInOutExpo'), function() {
 var colour = $(this).css('display');
 if (colour == 'none') {
 $('#2_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #description , #logo , #products , #links , #contact').css('display', 'block');
 } else {
 $('#2_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #description , #logo , #products , #links , #contact').css('display', 'none');
 }
 });
 });
 $("#3_open").click(function() {
 $("#description_container").slideToggle((500, 'easeInOutExpo'), function() {
 var description = $(this).css('display');
 if (description == 'none') {
 $('#3_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #logo , #products , #links , #contact').css('display', 'block');
 } else {
 $('#3_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #colourscheme , #logo , #products , #links , #contact').css('display', 'none');
 }
 });
 });
 $("#4_open").click(function() {
 $("#logo_container").slideToggle((500, 'easeInOutExpo'), function() {
 var logo = $(this).css('display');
 if (logo == 'none') {
 $('#4_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #products , #links , #contact').css('display', 'block');
 } else {
 $('#4_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #colourscheme , #description , #products , #links , #contact').css('display', 'none');
 }
 });
 });
 $("#5_open").click(function() {
 $("#products_container").slideToggle((500, 'easeInOutExpo'), function() {
 var products = $(this).css('display');
 if (products == 'none') {
 $('#5_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #logo , #links , #contact').css('display', 'block');
 } else {
 $('#5_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #colourscheme , #description , #logo , #links , #contact').css('display', 'none');
 }
 });
 });
 $("#6_open").click(function() {
 $("#contact_container").slideToggle((500, 'easeInOutExpo'), function() {
 var contact = $(this).css('display');
 if (contact == 'none') {
 $('#6_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #logo , #products , #links').css('display', 'block');
 } else {
 $('#6_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #colourscheme , #description , #logo , #products , #links').css('display', 'none');
 }
 });
 });
 $("#7_open").click(function() {
 $("#links_container").slideToggle((500, 'easeInOutExpo'), function() {
 var links = $(this).css('display');
 if (links == 'none') {
 $('#7_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #logo , #products , #contact').css('display', 'block');
 } else {
 $('#7_arrow').attr('src', './resources/images/icons/left.png');
 $('#background, #colourscheme , #description , #logo , #products , #contact').css('display', 'none');
 }
 });
 });
});
<div id="background">
 <div class="header" id="1_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="1_arrow">Background</div>
 <div id="background_container" class="containers" style="display:none;"></div>
</div>
<div id="colourscheme">
 <div class="header" id="2_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="2_arrow">Colour Scheme</div>
 <div id="colour_container" class="containers" style="display:none;"></div>
</div>
<div id="description">
 <div class="header" id="3_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="3_arrow">Description</div>
 <div id="description_container" class="containers" style="display:none;"></div>
</div>
<div id="logo">
 <div class="header" id="4_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="4_arrow">Logo and Icon</div>
 <div id="logo_container" class="containers" style="display:none;"></div>
</div>
<div id="products">
 <div class="header" id="5_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="5_arrow">Products</div>
 <div id="products_container" class="containers" style="display:none;"></div>
</div>
<div id="contact">
 <div class="header" id="6_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="6_arrow">Contact Information</div>
 <div id="contact_container" class="containers" style="display:none;"></div>
</div>
<div id="links">
 <div class="header" id="7_open">
 <img src="./resources/images/icons/right.png" class="arrow" id="7_arrow">Links</div>
 <div id="links_container" class="containers" style="display:none;"></div>
</div>

Jamal
35.2k13 gold badges134 silver badges238 bronze badges
asked Jun 22, 2015 at 0:35
\$\endgroup\$
1
  • \$\begingroup\$ have you tried using css? \$\endgroup\$ Commented Jun 22, 2015 at 0:36

2 Answers 2

2
\$\begingroup\$

You are repeating yourself too much. Not DRY at all. Read about Dry Principals

This snippet here could easily be extracted into a function, that then could be reused everywhere.

var background = $(this).css('display');
 if (background == 'none') {
 $('#1_arrow').attr('src', './resources/images/icons/right.png');
 $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
 } else {
 $('#1_arrow').attr('src', './resources/images/icons/left.png');
 $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
 }

You could create a function that would take as parameters everything that differ from each usage. Example-

function doStuff(selector, image1, image2, otherParam) {
 var background = $(this).css('display');
 if (background == 'none') {
 $(selector).attr('src', image1);
 $('#background, #colourscheme , #description , #logo , #products , #links , #contact').css('display', 'block');
 } else {
 $(selector).attr('src', image2);
 $('#colourscheme , #description , #logo , #products , #links , #contact').css('display', 'none');
 }
}
answered Jun 22, 2015 at 1:03
\$\endgroup\$
0
\$\begingroup\$

If you see a little your code, you will see that there are excessive repeated selectors, which (in most cases) means something goes wrong. Thinking how to improve it, finally I rewrote the entire code. The result (I hope it works):

$(function(){
 var targets = ['background', 'colour', 'description', 'logo', 'products', 'links', 'contact']; // containers
 var $elements = $('#background, #colourscheme, #description, #logo, #products, #links, #contact'); // jQuery elements
 var i, x; // we use `i` for the array and `x` for the event selector
 for( i = 0, x = 1; i < targets.length; i++ ){
 $('#' + x + '_open').on('click', function(){
 $('#' + targets[i] + '_container').slideToggle((500, 'easeInOutExpo'), function(){
 var visible; // will store the name of the method (show/hide)
 if( !$(this).is(':visible') ){
 $('#' + x + '_arrow').attr('src', './resources/images/icons/right.png');
 visible = 'show';
 }
 else{
 $('#' + x + '_arrow').attr('src', './resources/images/icons/left.png');
 visible = 'hide';
 }
 switch( x ){ // since the code is (almost) the same to all elements, we finally use the method which name was stored in `visible` variable
 case 1: visible == 'show' ? $elements.show() : $elements.not('#background').hide(); break; // this case is the only one that missmatch, so we use directly the method to show/hide
 case 2: $elements.not('#colourscheme')[visible](); break; // the rest of cases had the same syntax so it is easy to solve
 case 3: $elements.not('#description')[visible](); break;
 case 4: $elements.not('#logo')[visible](); break;
 case 5: $elements.not('#products')[visible](); break;
 case 6: $elements.not('#contact')[visible](); break;
 case 7: $elements.not('#links')[visible](); break;
 }
 });
 });
 x++;
 }
});

The first thing was to assign the names of the targets containers into an array and the elements into a jQuery object. Since we are working with an "ordered" identifiers (#1_open, #2_open...) we can assign the onclick event with a loop easily and work into it.

Since the code is almost the same, we just need to identify our current target x and proceed at convenience. There are more ways to do it, but well, this is just another one.

answered Jul 7, 2015 at 20:28
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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