Javascript pixel art maker project

Question

I am a newbie. I am trying to make a pixel art maker. I cannot figure out where I am going wrong at. It lets me choose the size and pick the color but when I hit enter to make the grid, it doesn't do anything. It just goes back to the original settings with 1 filled in and the color black. Any help would be appreciated.

let colorPicker = document.getElementById("colorPicker").value;
let height = document.getElementById("inputHeight").value;
let width = document.getElementById("inputWidth").value;
let table = document.getElementById("pixelCanvas");
let sizePicker = document.getElementById("sizePicker");

sizePicker.addEventListener('sumbit', function(event) {
  event.preventDefault()
  let height = document.getElementById("inputHeight").value;
  let width = document.getElementById("inputWidth").value;
  makeGrid(height, width);
});

function makeGrid(height, width); {
  let height = document.getElementById("inputHeight");
  let width = document.getElementById("inputWidth");
  table.innerHTML = null;
  for (let i = 0; i < height; i++) {
    let row = table.insertRow(i);
    for (let j = 0; j < width; j++) {
      let cell = row.insertCell(j);
      cell.addEventListener("click", function(event) {
        cell.style.backgroundColor = colorPicker.value;
      });
      cell.addEventListener("dblclick", function(event) {
        cell.style.backgroundColor = "";
      });
    }
  }
}

body {
  text-align: center;
}

h1 {
  font-family: Monoton;
  font-size: 70px;
  margin: 0.2em;
}

h2 {
  margin: 1em 0 0.25em;
}

h2:first-of-type {
  margin-top: 0.5em;
}

table,
tr,
td {
  border: 1px solid black;
}

table {
  border-collapse: collapse;
  margin: 0 auto;
}

tr {
  height: 20px;
}

td {
  width: 20px;
}

input[type=number] {
  width: 6em;
}

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Monoton">

<h1>Pixel Art Maker</h1>
<h2>Choose Grid Size</h2>
<form id="sizePicker">
  Grid Height:
  <input type="number" id="inputHeight" name="height" min="1" value="1"> Grid Width:
  <input type="number" id="inputWidth" name="width" min="1" value="1">
  <input type="submit">
</form>
<h2>Pick A Color</h2>
<input type="color" id="colorPicker">
<h2>Design Canvas</h2>
<table id="pixelCanvas"></table>

The `width` and `height` variables in the `makeGrid` function are not correct, they're referring to DOM nodes, not their values. — Etheryte, Sep 17 '22 at 16:28

score 0 · Answer 1 · edited Sep 17 '22 at 17:10

You have unnecessary errors. I fixed all of them in the snippet below.

Almost errors come from typo, redundant get values, passing wrong type parameters, and getting elements wrong moment.

Notable here is how you set empty backgroundColor. You should use transparent or inherit instead of "". Check this answer.

let table = document.getElementById("pixelCanvas");
let sizePicker = document.getElementById("sizePicker");

sizePicker.addEventListener('submit', function (event) {
    event.preventDefault();
    let height = document.getElementById("inputHeight").value;
    let width = document.getElementById("inputWidth").value;
    makeGrid(height, width);
});

function makeGrid(height, width) {
    table.innerHTML = null;
    for (let i = 0; i < height; i++) {
        let row = table.insertRow(i);
        for (let j = 0; j < width; j++) {
            let cell = row.insertCell(j);
            cell.addEventListener("click", function (event) {
                let colorPicker = document.getElementById("colorPicker").value;
                cell.style.backgroundColor = colorPicker;
            });
            cell.addEventListener("dblclick", function (event) {
                cell.style.backgroundColor = "inherit";
            });
        }
    }
}

body {
  text-align: center;
}

h1 {
  font-family: Monoton;
  font-size: 70px;
  margin: 0.2em;
}

h2 {
  margin: 1em 0 0.25em;
}

h2:first-of-type {
  margin-top: 0.5em;
}

table,
tr,
td {
  border: 1px solid black;
}

table {
  border-collapse: collapse;
  margin: 0 auto;
}

tr {
  height: 20px;
}

td {
  width: 20px;
}

input[type=number] {
  width: 6em;
}

<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Monoton">

<h1>Pixel Art Maker</h1>
<h2>Choose Grid Size</h2>
<form id="sizePicker">
  Grid Height:
  <input type="number" id="inputHeight" name="height" min="1" value="1"> Grid Width:
  <input type="number" id="inputWidth" name="width" min="1" value="1">
  <input type="submit">
</form>
<h2>Pick A Color</h2>
<input type="color" id="colorPicker">
<h2>Design Canvas</h2>
<table id="pixelCanvas"></table>

Keep learning and sharpen your skill.

Thank you for your assistance and advice. I was able to fix the issue and will focus more on learning. I know I have a long way to go. — Newbie82, Sep 18 '22 at 15:35

ThS · Answer 2 · 2022-09-18T07:53:20.513

Let's not focus on the typos on your code and just focus on the main issues.
The main issue you encounter is Uncaught SyntaxError: Identifier 'height' has already been declared error so let's investigate where it came from and why:

if we look closer we see that you have a lot of variables named the same in different place, you have height as a global variable and also you declared a variable with the same name, height, in the form submit listener.
This is not the source of the error thanks to the let keyword used to declare the height variable which declares a block-scoped local variable (source MDN). So with that being said, the issue is not coming from using the same name in various places. Let's dig deeper.
In the makeGrid function, you expect two variable that you called then height and width, it seems we're close! In that same function, we can say that the first line, let height = document.getElementById("inputHeight"), is the cause of Uncaught SyntaxError: Identifier 'height' has already been declared error but why ?
As a first look you may say "I didn't declare another variable with the same name and my height variable is local!" but wait, it seems we forgot that the function makeGrid expects a variable called height and that's the source of the error! height is declared on the function arguments and you trying to create a new variable that have the same name when let height = document.getElementById("inputHeight"); is executed.

The above explanation is true for the variable width so we should fix them both. The easiest solution is to choose different names but I'd rather refactor your code a bit and make some improvements so you get the most of my answer

Here's a live demo which contains a wealth of important comments that should help you along:

/** as a rule of thumb, always try to minimize calls to DOM related methods so in our case we cache the elements that we know that we will use them many time. This will improve our code memeory consumption */
const colorPicker = document.getElementById("colorPicker"),
  table = document.getElementById("pixelCanvas"),
  sizePicker = document.getElementById("sizePicker"),
  height = document.getElementById("inputHeight"),
  width = document.getElementById("inputWidth"),
  /** the function "makeGrid" was refactored (a bit) and now it doesn't need the "height" nor the "width" as it'll get those values on its own thanks to the cached variables height and width */
  makeGrid = () => {
    /** this function will use height and width variable to get their realtime values whenever the submit button is clicked. See the below loops declaration */
    table.innerHTML = '';
    for (let i = 0; i < height.value; i++) {
      const row = table.insertRow(i);
      for (let j = 0; j < width.value; j++) {
        const cell = row.insertCell(j);
        /** nothing fancy here just used an arrow function (one liner) */
        cell.addEventListener("click", () => cell.style.backgroundColor = colorPicker.value);
        cell.addEventListener("dblclick", () => cell.style.backgroundColor = "transparent"); /** or any other color you want */
      }
    }
  }

/** listen for submit events and then draw the grid by calling "makeGrid" */
sizePicker.addEventListener('submit', e => e.preventDefault() || makeGrid());

body {
  text-align: center;
}

h1 {
  font-family: Monoton;
  font-size: 70px;
  margin: 0.2em;
}

h2 {
  margin: 1em 0 0.25em;
}

h2:first-of-type {
  margin-top: 0.5em;
}

table,
tr,
td {
  border: 1px solid black;
}

table {
  border-collapse: collapse;
  margin: 0 auto;
}

tr {
  height: 20px;
}

td {
  width: 20px;
}

input[type=number] {
  width: 6em;
}

<!-- no changes made on the HTML nor the CSS part just skip to the JavaScript part -->
<link rel="stylesheet" href="https://fonts.googleapis.com/css?family=Monoton">

<h1>Pixel Art Maker</h1>
<h2>Choose Grid Size</h2>
<form id="sizePicker">
  Grid Height:
  <input type="number" id="inputHeight" name="height" min="1" value="1"> Grid Width:
  <input type="number" id="inputWidth" name="width" min="1" value="1">
  <input type="submit">
</form>
<h2>Pick A Color</h2>
<input type="color" id="colorPicker">
<h2>Design Canvas</h2>
<table id="pixelCanvas"></table>

The above demo is definitely not the only possible fix/solution. Also, I tried to keep things simple and understandable. Many possible improvements can be applied to the above demo like using Event Delegation instead of many listener on the table cells.

Learn more about let keyword on MDN.

Learn more about const keyword on MDN.

Javascript pixel art maker project

2 Answers2