Sidebar with slide-out feature

Question

I made a sidebar that has a slide-out feature. When you click on it, it shows more items. I made it with JS and HTML but I think the code can be done more efficiently. How can I clean the code up?

$('.slide-out').on('click', function() {
    let data = 'slideout-item';
    if($(this).data(data)) {
        let item = $('.slide-out-' + $(this).data(data));
        if(item.hasClass('slideout-active')) {
            item.hide("slide", { direction: "left" }, 250);
            item.removeClass('slideout-active');
            $('.slide-out-overlay').fadeOut()
        } else {
            item.show("slide", { direction: "left" }, 250);
            item.addClass('slideout-active');
            $('.slide-out-overlay').fadeIn()
        }
    }
});

$('.slide-out-overlay').on('click', function () {
    let items = ['content', 'system', 'account', 'other'];
    let item = '.slide-out-';
    for (i = 0; i < items.length; i++) {
        if($(item + items[i]).hasClass('slideout-active')) {
            $(item + items[i]).hide("slide", { direction: "left"}, 250);
            $(item + items[i]).removeClass('slideout-active');
            $('.slide-out-overlay').fadeOut();
        }
    }
});

<ul class="list-unstyled">
        <li class="sidebar-item title">{{ __('Information') }}</li>
        <li class="sidebar-item"><a href="{{ route('home') }}" class="sidebar-link {{ Route::currentRouteNamed('home') ? 'sidebar-active' : '' }}">{{ __('Dashboard') }}</a></li>
        <li class="sidebar-item title">{{ __('System') }}</li>
        <li class="sidebar-item"><a href="#" class="sidebar-link slide-out" data-slideout-item="content">Content<i class="far fa-chevron-right" style="position:absolute; right: 0;"></i></a></li>
        <li class="sidebar-item"><a href="#" class="sidebar-link slide-out" data-slideout-item="system">System<i class="far fa-chevron-right" style="position:absolute; right: 0;"></i></a></li>
        <li class="sidebar-item"><a href="#" class="sidebar-link slide-out" data-slideout-item="account">Account<i class="far fa-chevron-right" style="position:absolute; right: 0;"></i></a></li>
        <li class="sidebar-item"><a href="#" class="sidebar-link slide-out" data-slideout-item="other">Other<i class="far fa-chevron-right" style="position:absolute; right: 0;"></i></a></li>
</ul>

<div class="slide-out-block shadow slide-out-content" id="slide-out">
    <nav class="sidebar-slideout">
        <ul class="list-unstyled">
            <li class="sidebar-item title">{{ __('Content') }}</li>
            <li class="sidebar-item"><a href="{{ route('pages') }}" class="sidebar-link {{ Route::currentRouteNamed('pages') ? 'sidebar-active' : '' }}">{{ __('Pages') }}</a></li>
            <li class="sidebar-item"><a href="{{ route('blocks') }}" class="sidebar-link {{ Route::currentRouteNamed('blocks') ? 'sidebar-active' : '' }}">{{ __('Blocks') }}</a></li>
            <li class="sidebar-item"><a href="{{ route('layouts') }}" class="sidebar-link {{ Route::currentRouteNamed('layouts') ? 'sidebar-active' : '' }}">{{ __('Layouts') }}</a></li>
        </ul>
    </nav>
</div>

Answer 1

It has been a while since I worked with jQuery so I may not get specific API's correct.

1. You are repeating ("slide", { direction: "left"}, 250), if you ever changed one of the properties I would assume you'd want them to be consistent with each other. You can set an array then destructure that when calling show or hide. You can also apply this theory to your strings such as slide-out-overlay and slideout-active for example, if you find yourself typing the same strings out, generally you want to put them in a variable (ideally a const)

// OLD
$('.slide-out').on('click', function() {
    ...
    item.hide("slide", { direction: "left" }, 250);
    ...
});

$('.slide-out-overlay').on('click', function () {
    ...
    item.hide("slide", { direction: "left" }, 250);
});

// NEW
const toggleArguments ["slide", { direction: "left" }, 250]
$('.slide-out').on('click', function() {
    ...
    item.hide(...toggleArguments);
    ...
});

$('.slide-out-overlay').on('click', function () {
    ...
    item.hide(...toggleArguments);
});

2. It's common practice to use guard clauses, to reduce nesting. Closely related is early return's too, you should never really need the else statement.

// OLD
$('.slide-out').on('click', function() {
    let data = 'slideout-item';
    if($(this).data(data)) {
        let item = $('.slide-out-' + $(this).data(data));
        if (something) {
            ...
        } else {
            ...
        }
    }
});

// NEW
$('.slide-out').on('click', function() {
    let data = 'slideout-item';
    if (!$(this).data(data)) {
        return;
    }

    if (item.hasClass('slideout-active') {
        ...
        return;
    }

    // do code for !item.hasClass('slideout-active')
});

3. It is sometimes clearer to put a $ in front of variables that are jQuery objects

// OLD
let item = $('.slide-out-' + $(this).data(data));

// NEW
let $item = $('.slide-out-' + $(this).data(data));

4. In your .slide-out-overlay click function, you are finding .slide-out-overlay again within the function block, when you already have the element in this

// OLD
$('.slide-out-overlay').on('click', function () {
    ...
    $('.slide-out-overlay')...
});

// NEW
$('.slide-out-overlay').on('click', function() {
    $(this)...
});

5. In your .slide-out-overlay click function you are looping over any array of strings and then checking if the relevant element has class slideout-active. Could you not just look for elements that have slideout-active?

// OLD
$('.slide-out-overlay').on('click', function () {
    let items = ['content', 'system', 'account', 'other'];
    let item = '.slide-out-';
    for (i = 0; i < items.length; i++) {
        if($(item + items[i]).hasClass('slideout-active')) {
            ...
});

// NEW
$('.slide-out-overlay').on('click', function () {
    const $items = $('.slideout-active');
    $items.hide("slide", { direction: "left"}, 250); // .hide works on an array of elements, as do most jQuery methods
    ...
});

6. You seem to be using let where you can use const. Only ever use let if you are modifying the value later on. Otherwise use const

7. String interpolation is also an option to concatante your strings. Although with jQuery may look a little confusing

// OLD
let item = $('.slide-out-' + $(this).data(data));

// NEW
let item = $(`.slide-out-${$(this).data(data)}`);

I would also challenge you to write this without jQuery and use vanilla javascript and CSS transitions on the classes.

Hopefully that helps!