Streaming live at 10am (PST)

[RESOLVED] Improving my jQuery Code


#1

Dear JS Ninjas,

I wrote this for my website in Webflow.

$('#select-generic').click(function() {
	$('#info-generic').addClass('unhide').siblings().removeClass('unhide');
});
$('#select-survey').click(function() {
	$('#info-survey').addClass('unhide').siblings().removeClass('unhide');
});
$('#select-reg').click(function() {
	$('#info-reg').addClass('unhide').siblings().removeClass('unhide');
});
$('#select-exam').click(function() {
	$('#info-exam').addClass('unhide').siblings().removeClass('unhide');
});
$('#select-template').click(function() {
	$('#info-template').addClass('unhide').siblings().removeClass('unhide');
});

Basically I have 2 columns, first one being a <ul> and its <li> and the other a bunch of <div> wrapped inside a master <div>.

What I am trying to do is that, when user clicks <li#Apple>, it will add class "unhide" to <div#Apple> and remove "unhide" from all its siblings.

The code works well, but I'm wondering if anyone here could help me improve my code. I'm still learning now, and not really a code guy. But I would love to know how it can be improved.


#2

Without a link to your page we can't suggest anything.


#3

Hey Sam,

Due to NDA, I can't share any links to the site. The code piece is my only option. As you could see the way it was written is very redundant, I'm wondering any possibilities to make it more efficient or shorter.


#4

To me that code is fine, there's a saying that goes around in Software Development - KISS (Keep it Simple Stupid)

I can see exactly what it's doing, and there's nothing fancy so no need to bother with improving performance.
Thumbs up dude and good luck with your journey into the programming world! :smile:

NOTE: What does 'unhide' mean? (rhetorical)
I would rather use a convention that states the class's intention explicity like 'show' or 'visible'. By saying 'unhide' you are using a double negative

NOTE #2: There is a JQuery function called .show() and .hide(), check them out if you can


#5

Then at least post HTML structure of the elements that are referenced here, without any text.


#6
<div class="w-clearfix gm-new-form-content">
      <div class="gm-new-form-choice">
        <ul class="w-list-unstyled gm-new-form-list">
          <li id="select-generic" class="gm-new-form-item"></li>
          <li id="select-survey" class="gm-new-form-item"></li>
          <li id="select-reg" class="gm-new-form-item"></li>
          <li id="select-exam" class="gm-new-form-item"></li>
          <li id="select-template" class="gm-new-form-item"></li>
        </ul>
      </div>
      <div class="gm-new-form-info">
        <div class="gm-new-form-empty">
          <img src="https://d3e54v103j8qbb.cloudfront.net/img/image-placeholder.svg" class="gm-new-form-empty-thumb">
          <div class="gm-empty-title">Some gibberish here</div>
        </div>
        <div class="gm-absolute-block">
          <div id="info-survey" class="gm-new-form-info-overlay"></div>
          <div id="info-generic" class="gm-new-form-info-overlay"></div>
          <div id="info-reg" class="gm-new-form-info-overlay"></div>
          <div id="info-exam" class="gm-new-form-info-overlay"></div>
          <div id="info-template" class="gm-new-form-info-overlay"></div>
        </div>
      </div>
    </div>

I hope this suffice. Let me know if this helps. Thank you.


#7

Removed IDs from individual lis and divs. Added IDs to parent containers.

<div class="w-clearfix gm-new-form-content">
  <div class="gm-new-form-choice">
    <ul class="w-list-unstyled gm-new-form-list" id="links">
      <li class="gm-new-form-item"></li>
      <li class="gm-new-form-item"></li>
      <li class="gm-new-form-item"></li>
      <li class="gm-new-form-item"></li>
      <li class="gm-new-form-item"></li>
    </ul>
  </div>
  <div class="gm-new-form-info">
    <div class="gm-new-form-empty">
      <img src="https://d3e54v103j8qbb.cloudfront.net/img/image-placeholder.svg" class="gm-new-form-empty-thumb">
      <div class="gm-empty-title">Some gibberish here</div>
    </div>
    <div class="gm-absolute-block" id="targets">
      <div class="gm-new-form-info-overlay"></div>
      <div class="gm-new-form-info-overlay"></div>
      <div class="gm-new-form-info-overlay"></div>
      <div class="gm-new-form-info-overlay"></div>
      <div class="gm-new-form-info-overlay"></div>
    </div>
  </div>
</div>

jQuery is now this:

$('#links').on('click', 'li', function() {
  $('#targets').children().removeClass('unhide')
    .eq($(this).index()).addClass('unhide');
});

I'm not sure why you need to use the class 'unhide', if there is no animation involved, you can do away with adding and removing the classes, and use jQuery .show() and .hide()


#8

Another way to simplify it based on your initial method of using individual IDs:

$(.gm-new-form-item').click(function() {
  $('#info-'+ $(this).attr('id').match(/[a-z]+$/))
    .addClass('unhide').siblings().removeClass('unhide');
});

#9
$('#links').on('click', 'li', function() {
     $('#target').children().hide().eq($(this).index()).show();
});

#10

Wow awesome! @samliew and @Aidz, thanks a bunch guys! The code works, tested all of them. Besides some minor typo. :smiley: I'll rename the class as your suggested @Aidz.


#11

This topic was automatically closed 60 days after the last reply. New replies are no longer allowed.