I'd highly appreciate it if you can:
- Spot flaws, especially bad practices and suggest a possible alternative method.
- Spot inefficiencies and suggest how to perform that task efficiently.
- You may review my HTML/CSS as well.
var addRecordFormVisibility = false;
var addRecordForm = document.getElementById('addRecord');
var addRecordFormButton = document.getElementById('submit');
var errors = document.getElementById('errors');
var addRecordFormTrigger = document.getElementById('addRecordFormTrigger');
function prepare() {
addRecordForm.style.display="none";
}
function triggerAddRecordForm() {
if(addRecordFormVisibility === false) {
addRecordForm.style.display="block";
addRecordFormVisibility = true;
} else {
addRecordForm.style.display="none";
addRecordFormVisibility = false;
}
}
function submit(event) {
var userInput = document.getElementById('description').value;
errors.innerHTML = '';
if(userInput.length === 0) {
errors.innerHTML = "Please fill in the form!";
} else {
var listItem = document.createElement('li');
listItem.innerHTML = userInput;
document.getElementById('todoList').appendChild(listItem);
}
event.preventDefault();
}
document.addEventListener("DOMContentLoaded", prepare, false);
addRecordFormButton.addEventListener("click", submit, false);
addRecordFormTrigger.addEventListener("click", triggerAddRecordForm, false);#todoApplication {
width: 800px;
height: 100%;
margin: 0 auto;
background: #EEEEEE;
border: 1px solid #DDDDDD;
border-radius: 5px;
padding: 10px;
}
#errors {
color: red;
}<!DOCTYPE html>
<html lang="en">
<head>
<meta charset="UTF-8">
<title>JavaScript</title>
<link rel="stylesheet" href="src/style.css">
</head>
<body>
<div id="todoApplication">
<button id="addRecordFormTrigger">Trigger Add Record Form</button>
<div id="errors">
</div>
<form id="addRecord" method="POST">
<input id="description" type="text" name="description" placeholder="Enter the task here">
<button id="submit">Submit</button>
</form>
<ul id="todoList"></ul>
</div>
<script src="src/TodoApp.js"></script>
</body>
</html>-
\$\begingroup\$ Seeing what it does, I think "trigger" should be "toggle" instead ;-) \$\endgroup\$Mathieu Guindon– Mathieu Guindon2015年06月27日 14:51:18 +00:00Commented Jun 27, 2015 at 14:51
-
\$\begingroup\$ Yeah. I forgot that word. \$\endgroup\$Abandoned Account– Abandoned Account2015年06月28日 10:01:27 +00:00Commented Jun 28, 2015 at 10:01
1 Answer 1
Ok so, your code is overall looking nice, but a few changes could be made:
It's important to make sure you leave whitespace around your operators, like:
addRecordForm.style.display="none"; ^^
Could use whitespace around the operators.
I try not to suggest ternaries very often, but I believe that triggerAddRecordForm could be simplified with the use of one.
function triggerAddRecordForm() { if(addRecordFormVisibility === false) { addRecordForm.style.display="block"; addRecordFormVisibility = true; } else { addRecordForm.style.display="none"; addRecordFormVisibility = false; } }
into:
function triggerAddRecordForm() {
addRecordForm.style.display = addRecordFormVisiblity ? "none" : "block";
addRecordFormVisibility = !addRecordFormVisibility;
}
On the subject of triggerAddRecordForm(), in general you shouldn't compare to false, then else it. You should leave the check empty (checks if the variable is true) like so:
if(addRecordFormVisibility) {
doStuff();
} else {
doOtherStuff();
}
In submit(), you could improve errors.innerHTML = ''.
Why don't you just set errors to document.getElementById('errors').innerHTML instead, seeing as you don't use errors other than for that?
In submit() you should also improve your line spacing as well, as it seems a little strange.