2
\$\begingroup\$

I'm new to JavaScript and new to jQuery plugin development. My plugin is really easy I just want to taggify a <ul>

    (function($) {
        var allTags;
        $.fn.taggify = function(options) {
            allTags = this;
            _create(allTags);
            return this;
        };  

        function _create(theElement) {
            var inputWrapper = $('<li></li>');
            var input = $('<input></input>');
            input.attr('type', 'text');
            input.attr('autocomplete', 'off');
            input.css('border', 'none');
            input.css('outline', 'none');
            inputWrapper.html(input)

            allTags.append(inputWrapper);
            _event(input);
        }

        function _event(theText) {
            theText.keyup(function(e) {
                if(e.keyCode === 13) {
                    var aTag = $('<li></li>');
                    aTag.addClass('tag-choice');

                    var aSpan = $('<span></span>');
                    aSpan.addClass('tag-label');

                    var tagText= theText.val();
                    aSpan.html(tagText);
                    aTag.html(aSpan);

                allTags.prepend(aTag);
                theText.val('');
            }
        });
    }

})(jQuery);
\$\endgroup\$
1
\$\begingroup\$

Your code doesn't look too shabby! A few comments.

  • Declaring but then not using options made me stumble. If you don't have any options, don't make a parameter called options.

  • In my opinion there's not much point in separating _create and _event into two separate functions. If you do, you should rename _event so that it's more like a function name. Functions should be named such that they start with a verb; try something like _attachListener.

  • It's fine to use a closure-scope variable, but you're kind of half-using it. If you combine your two functions, you'll be able to get rid of it entirely.

  • _create isn't using its theElement parameter.

  • You should take advantage of method chaining when creating input. Also, consider naming jQuery variables with a leading $ to make it very explicit what type they are.

  • The .html() function does not take a jQuery object as a valid parameter; it takes either an htmlString or a function. Instead you should use inputWrapper.append(input);.

  • In fact, rather than creating an inputWrapper at all, use the .wrap() function.

  • Do you really want to use aSpan.html(tagText);, or do you want to use .text()? This depends on your use case but usually people will expect the behaviour of .text() (e.g. not having to escape their angle brackets).

  • For consistency, either name them both anInput and aSpan or drop the prefix.

  • Note that using function chaining, you can in fact completely eliminate the need for having any variables at all, if you prefer.

Here's my proposed rewrite:

(function($) {
  $.fn.taggify = function(options) {
    create(this);
    return this;
  };

  function create($theElement) {
    var $input = $('<input></input>')
      .attr('type', 'text')
      .attr('autocomplete', 'off')
      .css('border', 'none')
      .css('outline', 'none')
      .wrap('<li></li>');

    $input.on('keyup', function(e) {
      if (e.keyCode === 13) {
        var tagText = $input.val();
        var $span = $('<span class="tag-label"></span>');

        $span.text(tagText).wrap('<li class="tag-choice"></li>');
        $theElement.prepend($span.parent());
        $input.val('');
      }
    });

    $theElement.append($input.parent());
  }
})(jQuery);

$(function() {
  $('ul').taggify();
});
ul {
  background: #ccc;
  padding: 1em;
}
<script src="https://ajax.googleapis.com/ajax/libs/jquery/2.1.1/jquery.min.js"></script>
<ul></ul>

\$\endgroup\$
2
  • \$\begingroup\$ Thanks very much. But it looks like the wrap method doesn't work \$\endgroup\$
    – toy
    Jul 8 '15 at 3:50
  • \$\begingroup\$ @toy Sorry about that. Try it out now! :-) \$\endgroup\$
    – Schism
    Jul 8 '15 at 4:33

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.