0
\$\begingroup\$

I'm in the process of working on a client's site. I have a section containing a ul with list items and a div containing the slides that are triggered when clicking on an li item.

For instance, <li id="nav-one">Item One</li> triggers <div id="slide-one">...</div>.

Here is the jQuery portion:

var slidesNav = $('#slides-nav > li'),
    slidesContent = $('#slides-content').find('div'),
    id = '',
    slideId = '';

// Set initial state of slides to hidden, except the first one.
slidesContent.hide().first().slideDown();

var setClickEvents = function (id) {

    $('#nav-' + id).on('click', function () {
        slideId = $('#slide-' + id);

        if (slideId.css('display') !== 'block') {
            slidesContent.slideUp();
            slideId.slideDown();
        }
    });
};

for (var i = 0; i < slidesNav.length; i++) {
    id = $(slidesNav[i]).attr('id').substr('4');

    // assign click events
    setClickEvents(id);
}

If my solution can be improved, please explain as my ultimate goal is to become a better programmer.

Here is a jsbin with the full code. It's a variation of the original for the client.

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to codereview.stackexchange.com! You're more likely to receive an answer if you specify what you don't like about your current code or have a specific question about the code. \$\endgroup\$ – Emily L. Jun 21 '14 at 9:27
1
\$\begingroup\$

You could turn this into a general purpose plugin, but I'm not gonna go into that. There are also many good plugins that provide this functionality.

I'd ditch the IDs and just use the index of the current element as the reference.

You'll obviously still need a parent element with an ID, for nav and slides, so your code knows where they are.

Create class for active slide/nav, when a nav item is clicked, you get the index, multiply it by slide width/height, and animate. You get the corresponding slide with the index, so no need for IDs. Give the clicked element an active class. First step as soon as click happens would be to remove the active class so you can reassign it.

Then just put together some css using your parent elements class/ID.

Your code will be more generic, and your html will be cleaner.

\$\endgroup\$
  • \$\begingroup\$ Thanks for the help. I was thinking the nav and slides would have the same amount and assumed to be in the same order. I wasn't thinking about using just the index of the element. Good call. \$\endgroup\$ – ryentzer Jun 22 '14 at 16:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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