Tell me more ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 4 down vote favorite

I am quite new to javascript and I wonder if you experts can see any obvious mistakes (bad javascript coding) below? Or if there are any good improvements?

The code is working...

Noted improvements:

var images = iframe_images.contentWindow.document.getElementsByTagName('img'),
images_length = images.length;
for (var i = 0; i < images_length; i++) {

The code:

// add iframes to page
var normal_sortables = document.getElementById('normal-sortables');
normal_sortables.innerHTML = normal_sortables.innerHTML + '<div class="postbox"><iframe style="width:100%;height:300px;" id="iframe_upload" src="upload.php"></iframe><iframe style="width:100%;height:300px;" id="iframe_images" src="images.php"></iframe><iframe style="width:100%;height:300px;" id="iframe_pdf_documents" src="pdf.php"></iframe></div>';



// declaring all variables
var code_to_be_inserted = '',
textarea_content = document.getElementById('content'),
iframe_upload = document.getElementById('iframe_upload'),
iframe_images = document.getElementById('iframe_images'),
iframe_pdf_documents = document.getElementById('iframe_pdf_documents');



// upload iframes of images and pdf documents when file is uploaded
iframe_upload.onload = function () {
iframe_images.src = 'images.php';
iframe_pdf_documents.src = 'pdf.php';
}



// add image to content editor
iframe_images.onload = function () {
    var images = iframe_images.contentWindow.document.getElementsByTagName('img');
    for (var i = 0; i < images.length; i++) {
        images[i].onclick = function () {
            code_to_be_inserted = '<img alt="" src="'+this.src+'" />\n\n';
            textarea_content.value = code_to_be_inserted + textarea_content.value;
        }
    }
}



// add pdf documents to content editor
iframe_pdf_documents.onload = function () {
    var pdf_documents = iframe_pdf_documents.contentWindow.document.getElementsByTagName('a');
    for (var i = 0; i < pdf_documents.length; i++) {
        pdf_documents[i].onclick = function () {
            code_to_be_inserted = '\n\n<a href="' + this.href+'" target="_blank">Click here to open ' + this.innerHTML + '</a>';
            textarea_content.value = textarea_content.value + code_to_be_inserted;
            alert ('testar');
            return false;
        }
    }
}
share|improve this question
Hope you are also aware of jslint site which is good for syntactical checking. – NetEmp Dec 16 '11 at 10:20

1 Answer

up vote 3 down vote accepted

If you wanted to retain any event handlers that were already attached to your elements, you could change the first piece of code like this an avoid one of the dangers of setting innerHTML on an element that already has a bunch of HTML in it:

// add iframes to page
var normal_sortables = document.getElementById('normal-sortables');
// create container div
var newDiv = document.createElement("div");
newDiv.className = "postbox";
// put content into container div
newDiv.innerHTML = '<iframe style="width:100%;height:300px;" id="iframe_upload" src="upload.php"></iframe><iframe style="width:100%;height:300px;" id="iframe_images" src="images.php"></iframe><iframe style="width:100%;height:300px;" id="iframe_pdf_documents" src="pdf.php"></iframe>';
// add container div to the sortables
normal_sortables.appendChild(newDiv);
share|improve this answer
I see, that seams to be good practice :) Thanks – Hakan Dec 16 '11 at 2:05
Shouldn't it be: newDiv.className ="postbox"; – Hakan Dec 16 '11 at 2:16
@Hakan - Yes, that was a copy/paste error. Fixed. – jfriend00 Dec 16 '11 at 2:18

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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