6
\$\begingroup\$

This is a form generator I created so users can input a URL, description and link. The output will eventually be a JavaScript news slider. The HTML the form generated is irrelevant for now but I'd like to see how I can improve the JavaScript code handing the processing.

This is my first version/draft.

<style>

body{
    background-color:green;
}

.hideElement{
display:none;
}

.container{
display: inline-block;
margin:25px;
border: 2px solid white;
padding:10px;

}

p.slidenum{
text-align: center;
font-size: 22px;
font-weight: bolder;
}

p{
font-size: 20px;
}

input[type="text"]{
 background-color:white;
 color:black;
 font-size:16px;
 padding:10px;
}

input[type="button"]{
 background-color:#A17F3F;
 color:white;
 font-size:16px;
 border:2px solid;
 border-radius:5px;
 padding:10px;
}


</style>

<div id="step1" class=" ">
    <h1>How many slides do you want to generate?</h1>
    <input type="text" id="numOfSlides" name="numOfSlides">
    <input type="button" value="Continue" onClick="generateForm()">
</div>

<div id="step2" class="hideElement">
    <input type="button" id="genCodeButton" value="Generate Code" onClick="generateCode(); hideForm()">
    <input type="button" value="Go Back" onClick="hideStep1(); clearForm(); toggleStep2(); toggleCodeOutput(); showGenCodeButton()">
</div>

<!--empty div, will be populated once user enters num of slides -->
<div id="sliderFormGenerated" class=" "> </div>

<br>
<textarea id="codeOutput" class="hideElement" rows="30" cols="70"></textarea>






<script src="http://code.jquery.com/jquery-1.11.0.min.js"></script>
<script>


function generateForm(){
//get num of slides required by user, limit to 8
var numOfSlides = document.getElementById('numOfSlides').value;

//make sure num of slides is a valid integer 1-8
if(isNaN(numOfSlides)){
    alert("Well that doesn't make much sense. Please enter a positive integer 1-8");
    //document.getElementById('sliderFormGenerated').innerHTML="<p>Please enter a positive integer 1-8.</p>";
}
else if (numOfSlides >= 9) {

    alert("The limit is 8 slides, please enter 8 or less.");
    //document.getElementById('sliderFormGenerated').innerHTML="<p>Please enter a positive integer 1-8.</p>";
}
else if(numOfSlides < 1){

    alert("Please enter a positive integer 1-8");
    //document.getElementById('sliderFormGenerated').innerHTML="<p>Please enter a positive integer 1-8.</p>";
}
else {
    //document.getElementById('sliderFormGenerated').innerHTML="<p>Great! For simplicity, let's make one at a time</p>";
    //document.getElementById('sliderFormGenerated').innerHTML=sliderFormNumber;
    //for loop to add unique id to each generated element
    for(var i=0; i < numOfSlides; i++){

        //declare form to be inserted by fucntion call
        var sliderForm = " <p>\"Read More\" URL: </p><input type=\'text\' id=\'link\'>";


        $("#sliderFormGenerated").append("<div class=\"container\"><p class=\"slidenum\">Slide " +(i+1) + "</p><p>Image URL: </p> <input type=\'text\' id=\'imgsrc" + i + "\'" + ">" + i + "<p>Description: </p><input type=\'text\' id=\'description" + i + "\'>" + i +"<p>\"Read More\" URL: </p><input type=\'text\' id=\'link" + i +"\'>" +i + "</div>");

        $('#sliderFormGenerated').removeClass("hideElement");



    }
    toggleStep2(); 
    hideStep1();
}



}

function hideStep1(){
    $('#step1').toggleClass("hideElement");
}

function toggleStep2(){
    $('#step2').toggleClass("hideElement");
}

function showGenCodeButton(){
    $('#genCodeButton').removeClass("hideElement");
}

function hideForm(){
    $('#sliderFormGenerated').addClass("hideElement");
}

function toggleCodeOutput(){
    $('#codeOutput').addClass("hideElement");
}

function clearForm(){
    document.getElementById("sliderFormGenerated").innerHTML=" ";
}

function generateCode(){
    $('#codeOutput').toggleClass("hideElement");
    $('#genCodeButton').addClass("hideElement");

    //TODO: this is the same as numOfSlides variable declared above
    var slides = document.getElementById('numOfSlides').value;

    var inputArray = [];

    for (i = 0; i < slides; i++){

        var temp = document.getElementById('imgsrc' + i).value;
        var temp1 = document.getElementById('description' + i).value;
        var temp2 = document.getElementById('link' + i).value;

        inputArray.push("<img src=\'" + temp + "\'>" + "<p>" + temp1 + "</p>" + "<a href=\'"+temp2+"\'>Read More</a>");


    }


    document.getElementById('codeOutput').innerHTML=inputArray.join('');

}


</script> 
</body>
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

I will start with a snooty remark; please don't submit your first draft, but submit the best code you think you can write.

From a once over:

  • You should consider having your JavaScript in a separate file, JavaScript mixed in with your HTML is old skool
  • Indentation in function generateForm(){ is off
  • Do not use alert, it annoys users, the commented out approach of setting document.getElementById('sliderFormGenerated').innerHTML is superior
  • Do not keep commented out code
  • " <p>\"Read More\" URL: </p><input type=\'text\' id=\'link\'>"; -> I would prefer

    ' <p>"Read More" URL: </p><input type="text" id="link">';
    
  • onClick="hideStep1(); clearForm(); toggleStep2(); toggleCodeOutput(); showGenCodeButton()" <-You are lucky I was wearing goggles, this is terrible because
    • You are using onclick in the HTML tag -> please use addEventListener instead
    • If you had to use onclick, then you should at least have grouped these functions into 1 well named function, this way you would not have to create functions like showGenCodeButton() which is a short one liner that you only use once.
  • This:

    var temp = document.getElementById('imgsrc' + i).value;
    var temp1 = document.getElementById('description' + i).value;
    var temp2 = document.getElementById('link' + i).value;
    

    I am sure you could think up more creative names for those variable names

I have some more items, but at this point I think you need to polish your script and if you wish resubmit it, the quality of this code is very low.

\$\endgroup\$

Your Answer

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

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