1
\$\begingroup\$

I am working on a bookmarker project for replacing storage of bookmarks in a web browser. It will be storing bookmarks as per categories in a database. I have written some jQuery for the WIP search page.

A bit about functionality

  • There is a search box for categories with autocomplete which is done by my app
  • On clicking Enter category is added to the web page via this jQuery. There are checks whether the category is in the autocomplete and whether it is already present on page or not.

The JS file can be viewed on GitHub if anyone prefers that. Please review as I don't know about best practices of javascript and am using JQuery for the first time. This JS is associated with this HTML page. I am not placing the HTML page here as I am interested in Javascript review only. I am using Python 3.4.0 with Django 1.6.4.

Please tell if any other information required for review.

/**
 * Created by Aseem on 5/22/14.
 */

var cur_location = location.protocol + '//' + location.hostname + (location.port ? ':' + location.port : '');
var static_url = '/static/BookMarker';
var static_img_url = cur_location + static_url + '/img';

var category_input = $("#category_inp");

$(function () {
    category_input.autocomplete({
        source: "auto/"
    });
});

//Add Bindings for keys
category_input.keyup(function (e) {
    if (e.keyCode == 13) {
        $(this).trigger("enterKey");
    }
});

category_input.bind("enterKey", function (e) {

    var is_category = false;

    $(".ui-menu-item").each(function (index) {
            if (category_input.val() == $.trim($(this).text())) {
                is_category = true;
                return false;
            }
        }
    )

    if (is_category == false) {
        alert("Not a category");
        return false;
    }

    var already_present = false;

    $(".category").each(function (index) {
            if (category_input.val() == $.trim($(this).text())) {
                alert("Category already added");
                already_present = true;
                return false;
            }
        }
    )

    if (already_present == false) {
        $("#category_list").append(
            '<li class="category ' + category_input.val() + '">' +
                ' <img src="' + static_img_url + '/delete.png" class="delete-cat ' + category_input.val() + '" ' +
                ' title="Remove category ' + category_input.val() + '"/>' +
                category_input.val() +
                '</li>'
        );
    }
    category_input.val("");
});

$(document).on('click', ".delete-cat", function (e) {
    var classList = $(this).attr('class').split(/\s+/);
    $.each(classList, function (index, item) {
        if (item != 'delete-cat') {
            $('.category.' + item).remove();
        }
    });
})
\$\endgroup\$
1
\$\begingroup\$

I know that this comes a few years late and the code doesn't appear to exist on the github repository anymore. Perhaps you have learned much more about jQuery and the autocompletes since posting this but none-the-less here is a review of it.

It is great that the DOM reference for the category input (i.e. $("#category_inp")) is stored in a variable but others could be also - like the container for the list of selected categories (i.e. $("#category_list")).

Instead of binding the enterKey event to a callback, why not use the select event callback option? That way if the user selects an option with the mouse the callback will also be called.

And for the delete image click handler, it could be simplified using .parent():

$(document).on('click', ".delete-cat", function (e) {
    $(this).parent().remove();
});
\$\endgroup\$

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.