-5
\$\begingroup\$

I'd like shorten my jQuery. I know this is possible using this element and child but I don't know how it works.

I have an element being shown and hidden on click, basically. They all have the same ID, just different classes.

How is it possible to achieve this?

This is what my HTML looks like:

<div id="spotparent">

  <div id="spot"  class="one" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="one">
                    <img src="img/line-boost.png">

          </div>
        </div>

<div id="spot" class="two"  style="position: absolute; z-index: 103; width: 25px; height: 25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="two">
                    <img src="img/line-tpu.png">

          </div>
        </div>

          <div id="spot"  class="three" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="three">
                    <img src="img/line-wrap.png">

          </div>
        </div>

            <div id="spot"  class="four" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="four">
                    <img src="img/line-support.png">

          </div>
        </div>

         <div id="spot"  class="five" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="five">
                    <img src="img/line.png">

          </div>
        </div>

         <div id="spot"  class="six" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="six">
                    <img src="img/line-knit.png">

          </div>
        </div>


         <div id="spot"  class="seven" style="position: absolute; z-index: 103; width: 25px; height:25px;">
          <div id="ball"></div>
          <div id="pulse"></div>

          <div id="contentspot" class="seven">
                    <img src="img/line-signature.png">

          </div>
        </div>

      </div>

The point is on click of a spot id, the id 'contentspot' show ( by toggling a class who change the display from none to block).

for this I'm using the following JQuery:

$('#spot.one').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.one').toggleClass('showcontent');

});


$('#spot.two').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.two').toggleClass('showcontent');

});

$('#spot.three').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.three').toggleClass('showcontent');

});

$('#spot.four').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.four').toggleClass('showcontent');

});

$('#spot.five').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.five').toggleClass('showcontent');

});

$('#spot.six').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.six').toggleClass('showcontent');

});

$('#spot.seven').click(function() {
             // if($('.hide').hasClass('test')){

  $('#contentspot.seven').toggleClass('showcontent');

});

So it get repeated the all time.

I'm sure it's possible to make all this code in 2 lines. If anybody can explain the process for me to understand, it would be fantastic!

\$\endgroup\$

closed as unclear what you're asking by Ethan Bierlein, mdfst13, Jamal Jan 24 '17 at 2:00

Please clarify your specific problem or add additional details to highlight exactly what you need. As it's currently written, it’s hard to tell exactly what you're asking. See the How to Ask page for help clarifying this question. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 6
    \$\begingroup\$ Welcome to Code Review. Shortening jQuery code is too common of a request to be a useful title on this site. Please reticle your question to state what the code accomplishes, as stated in the How to Ask guidelines. Also, I suggest making a live demo by pressing Ctrl-M in the question editor. \$\endgroup\$ – 200_success Jan 23 '17 at 18:42
4
\$\begingroup\$

Before even looking at the JavaScript: You're misusing the id and class attributes!

IDs should preferably be unique across the whole document. But you have a whole bunch of elements all called spot. This is the exact wrong way to use IDs.

Conversely, class names are meant to be used for multiple elements that are in some way similar; elements that are of the same class. But here you're using different class names all the way through (one, two, etc.), negating the entire purpose of class names.

Also, class names (and IDs) should ideally be descriptive. Calling them one, two and so on doesn't explain anything about what they represent. If anything they tell you something about their order, but that's also not great because that's a presentation concern. Saying what order something goes in doesn't tell you much about what it represents or what makes it unique (and besides, their order is given from where they appear in the HTML).

The story repeats itself for the child elements, which also use IDs where they should use classes, and vice-versa. Again, this is exactly wrong.

You should at the very least swap the ID and class attributes around. What do the elements have in common? They're spots - so that's their class. In what way are they unique? Well, right now it's their ordering, so the IDs should be one, two, and so on. But, as mentioned, that's already given by their relative placement in the HTML. And simply numbering them isn't great, so you should find a more descriptive name if you can. (If you do need to number something, add an actual number, like foo1, foo2, and so on. That makes it a lot easier to handle in JavaScript, since you can do things like "#foo" + (n + 1) to dynamically reference the IDs. JavaScript doesn't speak English, so spelling out the numbers makes everything more difficult.)

However, you don't need to use IDs for anything here. Since all the elements should behave the same way, there's no reason to treat them as unique. They all look and behave the same, so they're all of the same class - at least as far I can tell.

Speaking of CSS, move that to a <style></style> element or separate file instead of having it in the body of the HTML. HTML provides structure, JS provides behavior, and CSS provides presentation. Ideally they're orthogonal, i.e. you'll be able to change one without affecting the others. But not if it's all a big jumble.

Anyway, the JS:

// attach a click handler to all elements of the 'spot' class
$('.spot').on('click', function () {
  // 'this' is the element that was click, so find its first
  // descendant of the 'content' class, and show it
  $(this).find('.content:first').toggleClass('showcontent');
});

Note here, that instead of contentspot I've just called it content. Calling it "content spot" is redundant, since it's already nested inside a spot element. So all in all your HTML should look more like this:

<div class="spot"> <!-- move CSS to a style-element or separate file -->
  <div class="ball"></div>
  <div clas="pulse"></div>
  <div class="content">
    <img src="img/line-boost.png">
  </div>
</div>

<!-- repeat as necessary -->

And you CSS (now separated from the body of the document) could look like:

.spot { // applies to all element with the "spot" class
  position: absolute;
  z-index: 103;
  width: 25px;
  height: 25px;
}

Also, I'm guessing here that the showcontent class (which is redundantly named) simply changes the display CSS attribute. If that's the case, you probably don't need the class, and can just use jQuery's toggle instead:

$('.spot').on('click', function () {
  $(this).find('.content:first').toggle();
});

In any case, no repetition of JS or CSS, and no need for IDs as far as I can tell.


Alternatively, you can add a class to the spot element itself, and use CSS to describe how that affects its descendants:

$('.spot').on('click', function () {
  $(this).toggleClass('active');
});

And CSS:

.spot .content {
  display: none; // content is normally hidden
}.

.spot.active .content {
  display: block; // content is shown if the spot has the 'active' class
}

This moves more of the presentation concerns to the CSS, where it belongs.

\$\endgroup\$
  • \$\begingroup\$ Hi Flambino, it makes all totally sense, thanks a lot !! I have 2 small issues if you can help me out ? When i click to one .spot, the .content display - fine after that If i click to another spot, I'd like the previous spot to close automatically. IS there a way around this ? As well I tried to change the click function by "hover" and it doesnt seems to work . .any idea ? Thanks for all your help !! \$\endgroup\$ – tibewww Jan 24 '17 at 10:46
  • \$\begingroup\$ @tibewww You could add a line to hide all the other .content-elements: var localContent = $(this).find('content:first'); $('.content').not(localContent).hide(); localContent.show(); For future reference, though, "how do I ...?" questions should go to StackOverflow; CodeReview is for already-working code \$\endgroup\$ – Flambino Jan 24 '17 at 10:57

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