2
\$\begingroup\$

So, I am trying to generate a web-page template with JavaScript according to user specified values. I tested it with just background-color and made it working.

I wanted to add a bunch of new elements. Also I need to add style to each elements. I am able to do it with the current code.

But the problem I am having is that the code is becoming "TOO HARD TO READ". And I don't think I'm writing clean code. So, the question I have for you fellow Dev's is that is there another way to achieve this - with just pure JavaScript (vanilla JS). That is:-

  • To add specific elements separately.
  • To style those elements also separately.

EDIT: I have updated the finished project. It's readable. But I would like to NOT use document.write as it is wasting time for me to read and understand. I would get stuck if I wanted to edit it later on... Please any other way you can suggest..

function renderForm(){
	var bgc = document.getElementById("bgcolor").value;
	var nc = document.getElementById("ncolor").value;
	var tc = document.getElementById("tcolor").value;
	var nm = document.getElementById("ntext").value;
	window.alert("Confirm To NUKE!");
	var generateTemplate = window.open("","_self");
	generateTemplate.document.write('<!DOCTYPE html><html><head><title>Incoming Nuke!</title></head><body style="background-color:'+bgc+'">');
	generateTemplate.document.write('<style>:root{ --main-nuke-color: '+nc+'; --main-text-color: '+tc+'; } .box{ position:absolute; display: block; height: 500px; width: 500px; animation-name: drop;animation-duration: 4s; animation-iteration-count: infinite;animation-direction:normal; animation-timing-function: linear; } .nhead{position: relative; display: block; height: 450px; width: 200px;border-radius: 50%; top: 20%; left: 15%; background-color:var(--main-nuke-color); transform: rotate(45deg); z-index: 9; } .nend{ position: absolute;display: block; width: 0; height: 0; border-top: 100px solid transparent;border-right: 100px solid  var(--main-nuke-color); border-bottom: 100px solid transparent; top: 20%; left: 50%; transform: rotate(-45deg); } .ntailleft{position: absolute; display: block; width: 0; height: 0; border-top: 80px solid transparent; border-right: 80px solid  var(--main-nuke-color);border-bottom: 80px solid transparent; top: 3%; left: 53%; transform:rotate(0deg); } .ntailright{ position: absolute; display: block; width: 0;height: 0; border-top: 80px solid transparent; border-right: 80px solid var(--main-nuke-color); border-bottom: 80px solid transparent; top: 23%; left:73%; transform: rotate(270deg); } .ntailmiddle{ position: absolute; display:block; width: 0; height: 0; border-top: 80px solid transparent; border-right:80px solid  var(--main-nuke-color); border-bottom: 80px solid transparent;top: 10%; left: 65%; transform: rotate(135deg); } .text{ position: absolute;display: block; font-size: 90px; transform: rotate(-90deg); top: 35%; left:-62%; color: var(--main-text-color); word-wrap: break-word; white-space: nowrap;  width: 430px;  overflow: hidden; text-overflow: ellipsis; }div.text:hover { overflow: visible; }@keyframes drop{ 0%{ top: -50%; left: 100%; } 100%{ top: 100%; left: -50%; } }</style>');	
	generateTemplate.document.write('<div class="box"><div class="nhead"><div class="text">'+nm+'</div></div><div class="nend"></div><div class="ntailleft"></div><div class="ntailright"></div><div class="ntailmiddle"></div></div>');	
	generateTemplate.document.write('</body></html>');
}	

function initMyEvents(){
  document.getElementById("nuked").onclick = renderForm;
}

window.onload = initMyEvents;
@import url('https://fonts.googleapis.com/css2?family=Geo&display=swap');
body{
	text-align: center;
	background-color: #cccccc;
	font-family: 'Geo', sans-serif;
	font-size: 30px;
}
.customizerTable{
	margin: auto;

}
.button{
	border: none;
 	align-items: center;
  	background-color: rgb(204,204,204);
    box-shadow: 2px 2px 4px 0 rgba(0, 0, 0, 0.25), -2px -2px 4px 0 rgba(255, 255, 255, 0.3);
  	border-radius: 50px;
  	display: flex;
  	justify-content: center;
    margin: auto;
    margin-left: 0;
    margin-top: 5%;
    padding: 5%;
 	width: 50%;
 	cursor: pointer;
 	outline: none;
 	text-decoration: none;
}
.button:active{
	box-shadow: -2px -2px 4px 0 rgba(255, 255, 255, 0.3) inset, 2px 2px 4px 0 rgba(0, 0, 0, 0.25) inset;
}
.card{
	border: none;
 	align-items: center;
  	background-color: rgb(204,204,204);
    box-shadow: 2px 2px 4px 0 rgba(0, 0, 0, 0.25), -2px -2px 4px 0 rgba(255, 255, 255, 0.3);
  	border-radius: 50px;
  	display: flex;
  	justify-content: center;
  	margin: auto;
  	padding: 10%;
 	width: 100%;
}
.customizeField{
	/*TEST 1*/
	position: absolute;
	top: 50%;
	left: 50%;
  	transform: translate(-50%, -50%);
  	/*TEST 1*/
}
.textbutton{
	border: none;
 	align-items: center;
  	background-color: rgb(204,204,204);
    box-shadow: -2px -2px 4px 0 rgba(255, 255, 255, 0.3) inset, 2px 2px 4px 0 rgba(0, 0, 0, 0.25) inset;
  	border-radius: 50px;
  	display: flex;
  	justify-content: center;
    margin: auto;
    margin-left: 0;
    margin-top: 5%;
    padding: 5%;
 	width: 50%;
 	cursor: pointer;
 	outline: none;
 	text-decoration: none;
}
<table class="customizeField">
			<tr>
				<td>
					<div class="card">
						<div>Customize</div>
						<table class="customizerTable">
							<tr>
								<td>Background color:</td>
								<td><input type="color" id="bgcolor" name="bgcolor" value="#80ccff" class="button"></td>
							</tr>
							<tr>
								<td>Nuke color:</td>
								<td><input type="color" id="ncolor" name="ncolor" value="#262626" class="button"></td>
							</tr>
							<tr>
								<td>Text color:</td>
								<td><input type="color" id="tcolor" name="tcolor" value="#e6e600" class="button"></td>
							</tr>
							<tr>
								<td>Enter Nuke Code:</td>
								<td><input type="text" id="ntext" name="ntext" maxlength="6" class="textbutton" value="#NUKED"></td>
							</tr>
						</table>
					</div>
				</td>
			</tr>
			<tr>
				<td>
					<a type="submit" value="Submit it!" class="button" id="nuked">NUKE!</a>
				</td>
			</tr>		
		</table>

\$\endgroup\$
5
  • \$\begingroup\$ To me, your code looks quite short and very easy to read. There are some minor best-practices that could be implemented, but nothing stands out as in serious need of improvement. Is this the actual full code you're using that you find too ugly? \$\endgroup\$ Commented May 7, 2020 at 11:28
  • \$\begingroup\$ Actually I have to start adding a lot more elements. I really needed to understand this before I re-code my entire script. \$\endgroup\$ Commented May 7, 2020 at 11:31
  • \$\begingroup\$ It's good to learn first, no? So, I can choose the best way possible. \$\endgroup\$ Commented May 7, 2020 at 11:32
  • 1
    \$\begingroup\$ Per the rules here, you should implement the code (even if it's ugly) for the multiple elements in order for it to be reviewed. It's not clear what you want with multiple elements either \$\endgroup\$ Commented May 7, 2020 at 11:33
  • \$\begingroup\$ Hey, sorry I'm late but. I didn't want to bother you since I actually completed this. But I would like to know if you could tell me another way to do this without using `document.write'. It really makes my code hard to edit, later. It's not a big project. But I want to know this. I don't like how I added all those CSS styles. If I wanted to change it, I might break everything. I want to share this code for any code newbie to understand. But right now it is not worth it, I guess... \$\endgroup\$ Commented May 10, 2020 at 3:03

1 Answer 1

Reset to default
1
\$\begingroup\$

The main issue that will make your code readable is to use template literals instead of ' and " string literals when constructing the HTML. Template literals allow for the writing of readable, multiline strings, as well as easy interpolation. For example, the following line:

document.write('<style>:root{ --someVar: ' + someValue + '}</style><div>foo</div><div>bar</div>');

can be written as:

document.write(`
  <style>
    :root {
      --someVar: ${someValue}
    }
  </style>
  <div>foo</div>
  <div>bar</div>
`);

By following this sort of pattern, your document.write strings become much nicer to read and comprehend.

Other possible improvements:

It's good to use informative variable names that give you a good sense of what the variable holds on sight. For example, if someone else saw the line

var tc = document.getElementById("tcolor").value;

They probably wouldn't have any idea what tc is supposed to be except by examining where else the tc variable is referenced.

Your current method of getting the value of each input separately is a bit WET, too. You might consider selecting all inputs in the document (or form) at once, somehow. One option is to use querySelectorAll, map each element to its value, then destructure into the variables:

const [
  backgroundColor,
  nukeColor,
  textColor,
  nukeCode,
] = [...document.querySelectorAll('input')].map(input => input.value);

That's short and reasonably elegant, but not entirely maintainable if you ever decide to remove/add an input or change their order. You could iterate over the inputs and put their values into an object instead:

const inputValues = {};
for (const input of document.querySelectorAll('input')) {
  inputValues[input.name] = input.value;
}

Once the user presses submit, they're presented "Confirm to nuke", but don't have any choice in the matter. Consider using window.confirm instead of window.alert to give them the option to cancel (or change the wording to "Nuke launching...").

You create a window with window.open and put into a variable named generateTemplate. That's a confusingly odd name for a window object - consider something like newWindow instead.

Creating a new window like this is a bit odd. If it's required for some reason, that's fine, but usually it would make more sense to stay on the current page and replace the necessary content.

It's usually a good idea to avoid assigning to onclick properties, because they only permit a single listener - if another script assigns to the onclick too, the earlier listener will be lost. Best to use addEventListener instead - even if you don't plan on adding additional listeners, it's a good habit to get into.

Same sort of thing for window.onload. Though, it'd probably be better to listen for the DOMContentLoaded listener, which doesn't wait for images and other media to load first - or, even better, have the initial JS run after the page is loaded to avoid having to handle any such events - either put the script in a separate file and give it the defer attribute:

<script src="myscript.js" defer>

Or put it at the bottom of the <body>:

  ... page content
  <script src="myscript.js">
</body>

document.write is weird to use nowadays. Better to use more trustworthy and modern methods of DOM manipulation, like with document.createElement or insertAdjacentHTML to insert elements. To set the document title, assign to myWindow.document.title.

Inside the created window, creating so much CSS dynamically is odd. If possible, use a separate file for the bulky CSS and link to it. This way, all you have to do dynamically is set the CSS variables:

myWindow.document.body.insertAdjacentHTML('beforeend', '<link rel="stylesheet" type="text/css" href="styles.css">');
myWindow.document.body.insertAdjacentHTML('beforeend', `
    <style>
    :root {
      background-color: ${inputValues.backgroundColor};
      --main-nuke-color: ${inputValues.nukeColor};
      --main-text-color: ${inputValues.textColor}
    }
    </style>
`);

All together, you get:

function renderNuke() {
  const inputValues = {};
  for (const input of document.querySelectorAll('input')) {
    inputValues[input.name] = input.value;
  }
  if (!window.confirm("Confirm To NUKE!")) return;
  const newWindow = window.open("", "_self");
  newWindow.document.title = 'Incoming Nuke!';
  newWindow.document.body.innerHTML = `
    <style>
    :root {
      background-color: ${inputValues.backgroundColor};
      --main-nuke-color: ${inputValues.nukeColor};
      --main-text-color: ${inputValues.textColor}
    }

    .box {
    position: absolute;
    display: block;
    height: 500px;
    width: 500px;
    animation-name: drop;
    animation-duration: 4s;
    animation-iteration-count: infinite;
    animation-direction: normal;
    animation-timing-function: linear;
  }

  .nhead {
    position: relative;
    display: block;
    height: 450px;
    width: 200px;
    border-radius: 50%;
    top: 20%;
    left: 15%;
    background-color: var(--main-nuke-color);
    transform: rotate(45deg);
    z-index: 9;
  }

  .nend {
    position: absolute;
    display: block;
    width: 0;
    height: 0;
    border-top: 100px solid transparent;
    border-right: 100px solid var(--main-nuke-color);
    border-bottom: 100px solid transparent;
    top: 20%;
    left: 50%;
    transform: rotate(-45deg);
  }

  .ntailleft {
    position: absolute;
    display: block;
    width: 0;
    height: 0;
    border-top: 80px solid transparent;
    border-right: 80px solid var(--main-nuke-color);
    border-bottom: 80px solid transparent;
    top: 3%;
    left: 53%;
    transform: rotate(0deg);
  }

  .ntailright {
    position: absolute;
    display: block;
    width: 0;
    height: 0;
    border-top: 80px solid transparent;
    border-right: 80px solid var(--main-nuke-color);
    border-bottom: 80px solid transparent;
    top: 23%;
    left: 73%;
    transform: rotate(270deg);
  }

  .ntailmiddle {
    position: absolute;
    display: block;
    width: 0;
    height: 0;
    border-top: 80px solid transparent;
    border-right: 80px solid var(--main-nuke-color);
    border-bottom: 80px solid transparent;
    top: 10%;
    left: 65%;
    transform: rotate(135deg);
  }

  .text {
    position: absolute;
    display: block;
    font-size: 90px;
    transform: rotate(-90deg);
    top: 35%;
    left: -62%;
    color: var(--main-text-color);
    word-wrap: break-word;
    white-space: nowrap;
    width: 430px;
    overflow: hidden;
    text-overflow: ellipsis;
  }

  div.text:hover {
    overflow: visible;
  }

  @keyframes drop {
    0% {
      top: -50%;
      left: 100%;
    }

    100% {
      top: 100%;
      left: -50%;
    }
  }
  </style>
  <div class="box">
    <div class="nhead">
      <div class="text">${inputValues.nukeCode}</div>
    </div>
    <div class="nend"></div>
    <div class="ntailleft"></div>
    <div class="ntailright"></div>
    <div class="ntailmiddle"></div>
  </div>
  `;
}

document.getElementById("nuked").addEventListener('click', renderNuke);
@import url('https://fonts.googleapis.com/css2?family=Geo&display=swap');
body {
  text-align: center;
  background-color: #cccccc;
  font-family: 'Geo', sans-serif;
  font-size: 30px;
}

.customizerTable {
  margin: auto;
}

.button {
  border: none;
  align-items: center;
  background-color: rgb(204, 204, 204);
  box-shadow: 2px 2px 4px 0 rgba(0, 0, 0, 0.25), -2px -2px 4px 0 rgba(255, 255, 255, 0.3);
  border-radius: 50px;
  display: flex;
  justify-content: center;
  margin: auto;
  margin-left: 0;
  margin-top: 5%;
  padding: 5%;
  width: 50%;
  cursor: pointer;
  outline: none;
  text-decoration: none;
}

.button:active {
  box-shadow: -2px -2px 4px 0 rgba(255, 255, 255, 0.3) inset, 2px 2px 4px 0 rgba(0, 0, 0, 0.25) inset;
}

.card {
  border: none;
  align-items: center;
  background-color: rgb(204, 204, 204);
  box-shadow: 2px 2px 4px 0 rgba(0, 0, 0, 0.25), -2px -2px 4px 0 rgba(255, 255, 255, 0.3);
  border-radius: 50px;
  display: flex;
  justify-content: center;
  margin: auto;
  padding: 10%;
  width: 100%;
}

.customizeField {
  /*TEST 1*/
  position: absolute;
  top: 50%;
  left: 50%;
  transform: translate(-50%, -50%);
  /*TEST 1*/
}

.textbutton {
  border: none;
  align-items: center;
  background-color: rgb(204, 204, 204);
  box-shadow: -2px -2px 4px 0 rgba(255, 255, 255, 0.3) inset, 2px 2px 4px 0 rgba(0, 0, 0, 0.25) inset;
  border-radius: 50px;
  display: flex;
  justify-content: center;
  margin: auto;
  margin-left: 0;
  margin-top: 5%;
  padding: 5%;
  width: 50%;
  cursor: pointer;
  outline: none;
  text-decoration: none;
}
<table class="customizeField">
  <tr>
    <td>
      <div class="card">
        <div>Customize</div>
        <table class="customizerTable">
          <tr>
            <td>Background color:</td>
            <td><input name="backgroundColor" type="color" value="#80ccff" class="button"></td>
          </tr>
          <tr>
            <td>Nuke color:</td>
            <td><input name="nukeColor" type="color" value="#262626" class="button"></td>
          </tr>
          <tr>
            <td>Text color:</td>
            <td><input name="textColor" type="color" value="#e6e600" class="button"></td>
          </tr>
          <tr>
            <td>Enter Nuke Code:</td>
            <td><input name="nukeCode" class="textbutton" value="#NUKED"></td>
          </tr>
        </table>
      </div>
    </td>
  </tr>
  <tr>
    <td>
      <a type="submit" value="Submit it!" class="button" id="nuked">NUKE!</a>
    </td>
  </tr>
</table>

\$\endgroup\$

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.