Show/Hide with plus minus

Question

So I've got it to work that it shows/hides the UL's/LI's, but I'm not sure what I'm doing incorrectly where it's not swapping out the +/- signs?

Here's my JS:

$(".top ul li:not(:has(li.current))").find("ul").hide().end() // Hide all other ULs
.click(function (e) {
if (this == e.target) {
    $(this).children('ul').slideToggle();
}
$(this).children("li.menu-item-has-children").text(this.toggle ? "-" : "+");
return false;
});

I have a class setup to append the li with a li:before that add the + sign before the li that has the nested ul's. But I'm not sure if I am going about it the right way to swap out the signs.

Here's the fiddle that I made:

http://jsfiddle.net/bc4mg13a/

Check http://stackoverflow.com/a/7886070/707636. Hope it'll help you to figure out the solution. — Bongs, Aug 21 '14 at 17:58
I posted an answer, but I recommend having a less verbose selector. In this case you have a class: `menu-item-has-children` that you can hook into such that attaching the click handler there will achieve the same effect if you instead use CSS to hide all the `sub-nav` ULs — Morklympious, Aug 21 '14 at 18:08
Thanks for the replies! Yeah, it was one of those pieced together bits of code that I'm trying to integrate into a wordpress sidebar menu. — ultraloveninja, Aug 21 '14 at 19:22

score 1 · Answer 1 · answered Aug 21 '14 at 18:06

1

Your code feels incredibly verbose. Well, at least your js. Here's a fiddle of your code that I modified a little bit.

Instead of hiding all your menus with js immediately on pageload, I applied a CSS display: none; to the sub-menu class:

.sub-menu {
  display: none; 
}

The js is cleaned up a bit, and since the click handler is bound to .menu-item-has-children, You're really only clicking on that to reveal the contained UL.

Give it a look. Hope it helps :)

answered Aug 21 '14 at 18:06

Morklympious

1,065
8
12

Thanks! Yeah, it was pieced together from some other bits of code I had found. So, it was one of those "Well I got this part to work, how about the other". That's the one thing with JS I feel that there is always more than one way to achieve the same result. – ultraloveninja Aug 21 '14 at 19:20
1

Definitely! I think I'm just a fan of clean code. It's one of those things where I look at it and go "Can I make this less wordy?" Because in the end I'll probably just clutter it with comments, but I'm glad that this could help you out :) – Morklympious Aug 21 '14 at 19:29
I was wondering, do you know how to get it to stop sliding if there is a link in one of the li's? I just noticed that and updated the fiddle. – ultraloveninja Aug 21 '14 at 19:57

score 1 · Accepted Answer · answered Aug 21 '14 at 18:09

1

There you go: http://jsfiddle.net/bc4mg13a/13/

$(".menu-item-has-children").on("click", function(e){
  e.stopPropagation();
  var clickedLi = $(this);
  $("> ul", clickedLi).slideToggle();
  clickedLi.toggleClass("current"); 
});

To start with, your first js line is a has so much redundant stuff.

$(".top ul li:not(:has(li.current))").find("ul").hide().end() // Hide all other ULs .click

could be:

$(".top ul li:not(.current)").find("ul").hide().end() // Hide all other ULs .click

On the other hand, i changed your code slightly, simplified your selectors. On each li click, i select direct ul children, and the i slidetoggle + toggle class the 'current' class.

i also switch the plus sign via the current class on css.

answered Aug 21 '14 at 18:09

Pablo Rincon

999
10
23

Also, stoppropagation will help to avoid the problem if you clicked one of the children li, for tha parent li to also receive the event. – Pablo Rincon Aug 21 '14 at 18:09
Thanks! This works well to, except even with the `e.stopPropagation();` it still makes the other children li's trigger the `slideToggle` – ultraloveninja Aug 21 '14 at 20:22
Really ? I added stopPorpagation exactly to avoid that happening. And on my side, when testing, only the clicked '+' opens. – Pablo Rincon Aug 21 '14 at 20:37
Huh. That's strange. Yeah, it keep triggering no matter what. I wonder if it's a browser thing. – ultraloveninja Aug 21 '14 at 20:40

score 0 · Answer 3 · answered Aug 21 '14 at 17:57

0

Simply add:

$(this).toggleClass('open');

To this:

if (this == e.target) {
    $(this).children('ul').slideToggle();
    $(this).toggleClass('open'); // <--
}

answered Aug 21 '14 at 17:57

beautifulcoder

10,832
3
19
29

score 0 · Answer 4 · answered Aug 21 '14 at 17:57

0

you can just add $(this).toggleClass('open'); before you return false but I would strongly look more into what your code is doing. I'm not so sure the line before is doing anything.

answered Aug 21 '14 at 17:57

user1964638

59
9

James Korden · Answer 5 · 2014-08-21T18:10:14.567

For how you have it set up, I would try...

    $(".top ul li:not(:has(li.current))").find("ul").hide().end() // Hide all other ULs
.click(function (e) {
    if (this == e.target) {
        $(this).toggleClass("open");
        $(this).children('ul').slideToggle();
    }
    return false;
});

Additional:

For formatting, you might want to do something like:

.menu-item-has-children {
    &:before {
        content:"+ ";
        color: $white;
        width: 10px;
        display:inline-block;
    }
}
.open {
    &:before {
        content:"- ";
        color: $white;
        width: 10px;
        display:inline-block;
    }
}

score 0 · Answer 6 · answered Aug 21 '14 at 17:59

Fixed JS:

$(".top ul li:not(:has(li.current))").find("ul").hide().end() // Hide all other ULs
.click(function (e) {
    if (this == e.target) {
        $(this).children('ul').slideToggle();
        $(this).toggleClass('open'); // added
    }
    return false;
});

Just added "$(this).toggleClass('open');" to use the class you specified in your CSS instead of trying to manipulate the text manually.

score 0 · Answer 7 · answered Aug 21 '14 at 17:59

0

you can do it like this and add $(this).toggleClass('open');

http://jsfiddle.net/bc4mg13a/5/

answered Aug 21 '14 at 17:59

Grasper

1,293
12
26

score 0 · Answer 8 · answered Aug 21 '14 at 18:15

0

You don't need to use text(this.toggle ? "-" : "+");

Inside the condition, just toggle the class .open that you have already defined in your SCSS/CSS.

Like so -

$(this).toggleClass('open');

JSFiddle

answered Aug 21 '14 at 18:15

aniskhan001

7,981
8
36
57

Show/Hide with plus minus

8 Answers8