4
\$\begingroup\$

It's a script that I've ripped off elsewhere, and have modified it via help from Stack Overflow on a couple of other questions, so that it tops out at 10 fields.

My knowledge of JS/jQuery is barely there. I'm terrified that I did it badly or wrongly. Could you please let me know if this is the correct way of doing it, or if there's another way that I should have done it differently?

I need to make sure that the max number of text fields created is limited to 10 at most.

Here's a demo and a JSFiddle.

jQuery(document).ready(function($) {
    var max_fields = 10; 
    var x = 1; 

    $('.my-form .add-box').click(function() {
        var n = $('.text-box').length + 1;

        if(x < max_fields) {
            var box_html = $('<p class="text-box"><label for="box' + n + '">Name <span class="box-number">' + n + '</span></label> <input type="text" name="on' + (n - 1) + '" value="" id="box' + n + '" /> <a href="#" class="remove-box">Remove</a></p>');
            box_html.hide();
            $('.my-form p.text-box:last').after(box_html);
            box_html.fadeIn('slow');
            x++; 
        }
    });

    $('.my-form').on('click', '.remove-box', function() {
        $(this).parent().css('background-color', '#FF6C6C').fadeOut("slow", function() {
            $(this).remove();
            $('.text-box').each(function(index) {
                var label = $(this).children('label');
                label.attr('for', 'box' + (index + 1));
                label.children('.box-number').text(index + 1);
                $(this).children('input').attr({
                    name: 'on' + index,
                    id: 'box' + (index + 1)
                });
            });
        });
        return false;
    });
});
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.3/jquery.min.js"></script>
<div class="my-form">
    <p class="text-box">
        <label for="box1">Name <span class="box-number">1</span></label>
        <input type="text" name="on0" value="" id="box1" />
        <a class="add-box" href="#">Add More Names</a>
    </p>
</div>

\$\endgroup\$
1
  • 4
    \$\begingroup\$ Your demo works fine for me. What are you worried about? Since JavaScript is client side, the user could create more than 10 fields if they really want to, and there's nothing you can do to stop that from happening. All you can do is do server side validation if that's going to be a serious problem. \$\endgroup\$ Commented Aug 27, 2015 at 18:38

1 Answer 1

3
\$\begingroup\$

Bored, so I made a few improvements as I mentioned (in the comments that have vanished when this was moved to Code Review):

Use the jQuery shortcut DOM-ready handler

A handy short-cut for your DOM ready, with locally scoped $ is just jQuery(function($){...});

Store templates in HTML, not JavaScript strings:

<script id="template" type="text/template">
    <p class="text-box">
       <label for="box{n}">Name <span class="box-number">{n}</span></label> <input type="text" name="on{-1}" value="" id="box{n}" /> <a href="#" class="remove-box">Remove</a>
    </p>
</script>

and access like this:

var box_html = $('#template').html().replace(/{n}/g, n).replace(/{-1}/g, n-1);
var $box = $(box_html);

Count the current items when you need them

var n = $('.text-box').length + 1;
if (n <= max_fields) {

Restrict to the current "group" of inputs:

var $form = $(this).closest('.my-form');
$('p.text-box:last', $form).after($box);

This also means it can now handle multiple sets of inputs, each with different name/id prefixes.

Combine operations when you can:

$box.hide().fadeIn('slow');

Putting it all together:

JSFiddle: http://jsfiddle.net/TrueBlueAussie/q2emvzdd/4/

Notes:

  • A prefix was needs to be supplied by the parent form (e.g. a data attribute) so that each set of inputs has unique IDs and names. This is not applied to the first entry yet, but I have to go now, so something for you to finish :)
\$\endgroup\$

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.