0
\$\begingroup\$

I created jQuery code to automatically insert new input field when the previous field is focused. The code works well but seems rather lengthy to me.

What I'm currently doing is cloning an element and then modifying the clone's attributes. As I add new a element into a DOM, I also need to modify this element and increase its value attribute by 1:

<input type="hidden" id="id_cat-TOTAL_FORMS" value="1" name="cat-TOTAL_FORMS">

How could this be improved?

jQuery:

var slist = {
    'init': function () {
        $('form#cat-form').on('focus', 'p:last input[type="text"]', slist.addCategoryForm);
    },

    'addCategoryForm': function() {
        slist.totalcatforms = parseInt($('form#cat-form input[id$=TOTAL_FORMS]').attr('value'));
        console.log(slist.totalcatforms);
        $('form#cat-form input[id$=TOTAL_FORMS]').attr('value', slist.totalcatforms + 1);
        console.log('TOTAL_FORMS = ' + parseInt($('form#cat-form input[id$=TOTAL_FORMS]').attr('value')));
        $('form#cat-form p:first').clone().insertBefore('form#cat-form input[type="submit"]');
        $('form#cat-form p:last label').attr('for', 'id_cat-' + slist.totalcatforms + '-name');
        $('form#cat-form p:last input[type="text"]').val('').attr({
            id: 'id_cat-' + slist.totalcatforms + '-name',
            name: 'cat-' + slist.totalcatforms + '-name'
            });
        $('form#cat-form p:last input[type="hidden"]').attr({
            id: 'id_cat-' + slist.totalcatforms + '-id',
            name: 'cat-' + slist.totalcatforms + '-id'
            });

    }
}

$(document).ready(slist.init);

HTML:

<form id="cat-form" method="post" action="/" name="cat-form">
<div style="display:none">
  <input type="hidden" value="hjYpU5a6AD7VxhZpSSQv3s3efJ7uh2Z8" name=
  "csrfmiddlewaretoken">
</div>

<input type="hidden" id="id_cat-TOTAL_FORMS" value="1" name=
"cat-TOTAL_FORMS">
<input type="hidden" id="id_cat-INITIAL_FORMS" value="0" name=
"cat-INITIAL_FORMS">
<input type="hidden" id="id_cat-MAX_NUM_FORMS" name=
"cat-MAX_NUM_FORMS">

<p>
    <label for="id_cat-0-name">Name:</label>
    <input type="text" maxlength="30" name=
"cat-0-name" id="id_cat-0-name">
    <input type="hidden" id="id_cat-0-id" name=
"cat-0-id">
</p>
<input type="submit" value="Save" name="catForm">
</form>
\$\endgroup\$

2 Answers 2

1
\$\begingroup\$

Here's a quick review of your JavaScript code:

  1. Please remember to always cache your selectors.

  2. To get the value of an input element, use .val() instead of .attr('value').

  3. There's no need to use two separate steps to first get the value, then set it. Instead, pass a function to val, and return the new value you want.

  4. To convert a string to a number, just prefix it with the plus sign (e.g. +'32' will return the number 32). It's safer, and more concise, than parseInt.

  5. When using CSS selectors, you should always strive to use the native CSS3 selectors, since they're much faster than jQuery's own custom selectors. :first and :last are not CSS3 selectors. In your case, you should use :first-of-type and :last-of-type, since they're native CSS3 selectors.

  6. Since IDs are unique per page, there's no reason to qualify an ID selector with the tag name (as you've done with form#cat-form). Just use the ID on its own. Again, better performance.


With all that in mind, here's some sample code for you:

var slist = {
    cache: {},
    init: function () {
        slist.cache.form        = $('#cat-form');
        slist.cache.totalsInput = slist.cache.form.find('input[id$=TOTAL_FORMS]');
        slist.cache.toClone     = slist.cache.form.find('p:first-of-type');

        slist.cache.form.on('focus', 'p:last-of-type input[type="text"]', slist.addCategoryForm);
    },
    addCategoryForm: function() {

        var totalCount;
        slist.cache.totalsInput.val(function (i, v) {
            totalCount = +v;
            return totalCount++;
        });

        slist.cache.toClone.clone()

            .find('label')
                .prop('for', 'id_cat-' + totalCount + '-name')

            .end().find('input[type="text"]')
                .val('').prop({
                    id: 'id_cat-' + totalCount + '-name',
                    name: 'cat-' + totalCount + '-name'
                })

            .end().find('input[type="hidden"]')
                .prop({
                    id: 'id_cat-' + totalCount + '-id',
                    name: 'cat-' + totalCount + '-id'
                })

            .end().insertAfter( $(this).closest('p') );
    }
}

$(slist.init);
\$\endgroup\$
1
\$\begingroup\$

Here's a short demo, and the code is commented to reflect my take on improvements

HTML

<form id="cat-form" method="post" action="/" name="cat-form">
    <!--
    our template
    this is fetched as a string, and converted into HTML by jQuery later on
    this does not execute since the type "text/template" is unknown to the browser
    -->
    <script type="text/template" id="nameTemplate">
        <li>
            <input type="hidden" id="id_cat-{name}-id" name="cat-{name}-id">
            <label for="id_cat-{name}-name">Name:</label>
            <input type="text" id="id_cat-{name}-name" name="cat-{name}-name" maxlength="30">
        </li>
    </script>
    <div style="display:none">
        <input type="hidden" value="hjYpU5a6AD7VxhZpSSQv3s3efJ7uh2Z8" name="csrfmiddlewaretoken">
    </div>
    <!-- 
    i would use IDs rarely, only when elements are unique.
    for form elements, i would stick to names instead
    -->
    <input type="hidden" name="cat-TOTAL_FORMS" value="0">
    <input type="hidden" name="cat-INITIAL_FORMS" value="0">
    <input type="hidden" name="cat-MAX_NUM_FORMS">
    <!--
    create a container for all the names
    since they are somewhat related to eachother, we use a list
    we also avoid using insertBefore(submitButton)
    -->
    <ul class="id_cat-names"></ul>
    <input type="submit" value="Save" name="catForm">
</form>​​​

JS

$(function() {

        //IDs are unique. No need to be so specific on the selector.
        //we take form as our "base" to find the other elements.  
    var form = $('#cat-form'),
        //look for the name list in context of the form
        //this totally works like form.find('.id_cat-names'), only shorter
        nameList = $('.id_cat-names',form),
        //cache the total forms element, finding it using the same method
        totalForms = $('input[name="cat-TOTAL_FORMS"]', form), 
        //we get the template text
        //this is very useful for creating multiple elements of the same internals
        template = $('#nameTemplate', form).html(), 
        //track latest count, initially based on totalForms value
        latestCount = totalForms.val();

    function addCategoryForm(){
        //we then take the template text
        //replace the numbers with an incremented count
        //turn it into HTML using jQuery
        //append it to nameList
        $(template.replace(/\{name\}/g,++latestCount)).appendTo(nameList);
        //update latest count
        totalForms.val(latestCount);
    }

    //focus event handler for the last name field
    //quotes are optional on single word element types
    nameList.on('focus', 'li:last input[type=text]', function(){
        addCategoryForm();
    });

    addCategoryForm();

}); ​
\$\endgroup\$
1
  • \$\begingroup\$ Very interesting! A lot of new things for me in your approach. Thanks, I really appreciate your feedback! I think I won't be able to use it "as is" because I can't modify the HTML output that much. A lot of it is generated by the framework (Django). For example Django relies on element ids when processing forms, so they must stay. In any case, your response is fantastic for learning purposes. Thanks again. \$\endgroup\$
    – finspin
    Dec 2, 2012 at 19:08

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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