1
\$\begingroup\$

I've been getting help with this script seen below. It allows one input box to be used to put values into a text area and also display a list of the items. The items can be deleted once inserted:

http://jsfiddle.net/ZTuDJ/50/

// If JS enabled, disable main input
$("#responsibilities").prop('disabled', true);
// $("#responsibilities").addClass("hidden");

// If JS enabled then add fields
$("#resp").append('<input placeholder="Add responsibility" id="resp_input" ></input><input type="button" value="Add" id="add"> ');

// Add items to input field
var eachline='';
$("#add").click(function(){
    var lines = $('#resp_input').val().split('\n');
    var lines2 = $('#responsibilities').val().split('\n');
    if(lines2.length>10)return false;
    for(var i = 0;i < lines.length;i++){
        if(lines[i]!='' && i+lines2.length<11){
        eachline += lines[i] + '\n';
        }    
    }

    $('#responsibilities').text($("<div>" + eachline + "</div>").text()).before("<li>"+$("<p>"+lines+"</p>").text()+"<span class='right'><a href='#'>Remove</a></span></li>");

    $('#resp_input').val('');
});    

$(document).on('click', '.right a', function(){
    var el = $(this).closest('li')
    var index = $('li').index(el);
    var text = eachline.split('\n');
    text.splice(index, 1);
    eachline = text.join('\n')
    $('#responsibilities').text(eachline)
    el.remove()
})

I was wondering if it would be possible to tag links in the list with the following information:

<span refid="1">Remove</span>

By tagging the links in this way I was thinking I could easily point to them with jquery if I wanted to delete it or later edit it.

Is there room to simplify this script in this way?

\$\endgroup\$
7
  • \$\begingroup\$ May I suggest a solution that uses another library in place of jQuery, or just adds another library? There are very good tools that make this very easy to do. \$\endgroup\$ – Benjamin Gruenbaum Jun 18 '13 at 21:21
  • \$\begingroup\$ I really don't want to use any other frameworks other than jQuery I'm afraid. \$\endgroup\$ – J.Zil Jun 18 '13 at 21:22
  • \$\begingroup\$ What about dropping the jQuery dependency? \$\endgroup\$ – Benjamin Gruenbaum Jun 18 '13 at 21:23
  • \$\begingroup\$ Straight javascript? \$\endgroup\$ – J.Zil Jun 18 '13 at 21:23
  • 1
    \$\begingroup\$ Yes, it's very hacky :) I'm working on something \$\endgroup\$ – Benjamin Gruenbaum Jun 18 '13 at 21:32
2
\$\begingroup\$

What's good here:

  • It's fairly simple
  • Using prop over attr

What I did not like

  • Using HTML strings in code makes code very hard to modify later on. It's hard to debug and might cause potential issues (like making XSS easy for example).
  • No separation of concerns. You're treating your HTML like your source of knowledge instead of backing up your data with a model in the background. This is very harmful and I personally consider it a big design flaw. Your business logic (in this case the list items) and your presentational layer (the DOM nodes) is the same here. Every time you want to operate on the list you have to query the DOM. This is not only slow, but it becomes very nasty very fast as your code logic begins to expand.

There are frameworks like KnockoutJS that do data binding making this trivial to accomplish in just a few lines of code. However since you wanted a solution that has no 'magic' (Which I totally appreciate by the way) let's see what would be a simple JavaScript solution would look like.

Short note, I'm doing JavaScript with function constructors here since a lot of people find it easier, personally I'd use object initializers.

Here is a working fiddle

First, we'll have a data model for a responsibility.

//This is our element in the view model.
function Responsibility(text){
    this.text=text;
}

Next, we'll need to store all our responsibilities. We'll use an array:

// a list of our responsibilities, an actual view model to back our data
var responsibilities = []; 

Now, we'll want a render function, that'll take our elements, and turn them into actual DOM objects. We'll spit it into two. First, we'll render the list item:

function renderResponsibility(rep,deleteClick){
    var el = document.createElement("li");
    var rem = document.createElement("a");
    rem.textContent = " Remove";
    rem.onclick = deleteClick;
    var cont = document.createElement("span");
    cont.textContent = rep.text;
    el.appendChild(cont);
    el.appendChild(rem);
    return el;
}

On renderResponsibility another alternative would be to use a templating engine, but you said no extra libraries so I'm keeping my word :)

Now, our general render:

//render our actual elements
function render(){
    respList.innerHTML = "";
    // note, foreach needs a modern browser but can easily be shimmed
    responsibilities.forEach(function(responsibility,i){
        var el = renderResponsibility(responsibility,function(){
            responsibilities.splice(i,1);//remove the element
            render();//re-render
        });
        respList.appendChild(el);
    });
    //update the text area;
    respTextArea.textContent = responsibilities.map(function(elem){
       return elem.text;//get the text. 
    }).join("\n");
}

Finally, let's add the functionality to the Add button:

//events 
addButton.onclick = function(e){
    var text = newResp.value;
    var resp = new Responsibility(text);
    responsibilities.push(resp);
    render();
    newResp.value = "";
}

And we're done :)

Just to tease:

Also here is a more 'jQuery' version of the vanilla solution. As you can see a DOM manipulation library isn't very useful when building an application.

\$\endgroup\$
6
  • \$\begingroup\$ Thank you Benjamin, that was extremely informative. I will read through this and make sure I understand each line. Thank you again for your help, I really appreciate it \$\endgroup\$ – J.Zil Jun 18 '13 at 22:02
  • 1
    \$\begingroup\$ @JamesWillson I'm glad I could help, if you'd like to discuss this further (I really can not express how important separation of concerns is) you're welcome to the javascript chat in SO, it's how I got to this question in the first place. Here is the KnockoutJS solution, jsfiddle.net/sGjq8 I think it's much cleaner and much nicer than the alternatives. Frameworks like KnockoutJS, EmberJS and AngularJS are much better suited at building web applications than libraries like jQuery. \$\endgroup\$ – Benjamin Gruenbaum Jun 18 '13 at 22:07
  • \$\begingroup\$ Can you please tell me your aversion to using jQuery please? I am forced to have jquery on my site due to google maps and certain other services so I'd like to keep everything in jquery but not at the expense of making the code horrible... \$\endgroup\$ – J.Zil Jun 18 '13 at 22:43
  • 1
    \$\begingroup\$ I have no problem with jQuery itself (did I say that?). I use jQuery myself a lot, especially for legacy browser support and animations. Query selectors are practically useless (you created the elements, why would you ask for them? You own them). Creating elements with text seems counter productive. Working with the DOM api is usually a lot cleaner than doing string appends in jQuery. Basically in your use case it offers no real advantage. \$\endgroup\$ – Benjamin Gruenbaum Jun 18 '13 at 22:54
  • \$\begingroup\$ Ok, I didn't mean to put words in your mouth, but it did sound like you didnt think jquery was the ticket, and I am just trying to understand why. I'd be really interested to see how the jquery version compared it something like the knockoutjs solution. I'll be doing some more reading on all of this thanks to you \$\endgroup\$ – J.Zil Jun 18 '13 at 22:57

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.