Use prettier and eslint
Code auto-formatters are awesome. Try one at https://prettier.io/playground and look into adding it to your build which will make your code easier for other developers to read.
Linters automatically tell you about many problems in your code. A good portion of code reviews on this site are pretty much repeating linter output. Try eslint at https://eslint.org/play/.
Use const rather than let
In general, never use let unless you really need to mutate something, like a loop counter. Otherwise, use const. const guarantees the variable will never be reassigned, making the code easier to read (one less possible state change to reason about) and avoids a whole class of potential bugs. A codebase chock-full of lets is a sure sign of other problems.
Avoid pointless intermediate variables
Minor point, but
function getInput() {
let promptInput = prompt('Enter a whole number between 1 and 100', '');
return promptInput;
}
could be
const getInput() => prompt('Enter a whole number between 1 and 100');
Even so, prompt is generally considered to provide a poor UX, so I'd use an <input type="number" min="1" max="100"> element.
I prefer arrow functions but the classic function syntax with return is acceptable too. What I like about the const/arrow function version:
- Guarantees no reassignments
- Guarantees the function is only visible below its definition like a normal variable; no magic hoisting
- Makes it obvious that when you do use
function, it's because you want to avoid automatic this binding
- No need to use
return much of the time, encouraging short functions
- Less typing; cleaner-looking, modern functional style that is more aesthetically pleasing to me.
I don't think you need to specify the second default value argument to prompt if it's just an empty string.
Perform input conversions
prompt() always returns a string, and JS is "smart" enough to parse strings to integers when using the * operator, but it's best to make that conversion by hand and create a number as soon as possible after receving the input. This separates presentation and logic, making refactoring and debugging easier and helps mitigate surprising type bugs, which JS is especially prone to. Implicit conversion is generally seen as a flaw in JS' design rather than a feature. Hence TypeScript.
Validate input
UX: if the user enters invalid input, nothing happens. I suggest re-prompting the user when they enter something invalid.
It's also not entirely clear to what the number the user is entering represents exactly, based on the prompt. Maybe explain that it's the grid side length.
Remove debugging logs and commented-out code
console.log(event.target); spams the console when I run your snippet. These logs slow down the app and are unsightly.
Similarly, commented-out code shouldn't be committed or pushed to production.
Use high-level JS array functions
Avoid counter-based for loops in code. There's almost always more idiomatic way.
function createArray(promptInput){
const square = [];
for(i = 0; i < promptInput; i++) {
square[i] += `${i}`;
}
return square;
}
could be
const createArray = n => [...Array(n)].map((_, i) => `${i}`);
There's a deeper reason for doing this other than aesthetics--your loop counter i creates a global variable attached to the window which could easily interfere with code elsewhere. Always "use strict"; at the top of your scripts to help avoid such errors. With traditional for loops, it's all too easy to mess up the index or .length and introduce bugs. A classic bug is typoing .lenght.
Also, traditional for loops don't self-document the code--it's hard to tell whether they're acting as filters, mappers, reducers, searchers, existence checks or initializers at a glance.
Notice that the function name, createArray, sounds pretty generic, but it then has a confusing promptInput parameter that doesn't make much sense taken out of context and implicitly couples it to the code around it. I'd just call this parameter n. Also, it doesn't just create any old array, it creates an array of string numbers from 0..n. Perhaps createAscendingNumberArray is a more precise name.
Don't generate dynamic classes or ids
div.classList.add(`box${createUniqueClass()}`) and the createUniqueClass are antipatterns. Rather than using dynamic classes, just add a single .box class and select the n-th item of elements matching that class if you need to. But when you're creating dynamic elements, you can just track them in an in-memory data structure so you don't have to select them. This helps reduce two-way data flow in the program.
Event delegation is a useful way to figure out which element in a grid of children had the event triggered on it.
Separate CSS and JS
Your code leaks too much CSS into the JS logic.
For example,
container.setAttribute('style', 'display: flex; flex: 1 1 0; height: 960px; width: 960px; margin: auto; border: solid black 1px; flex-wrap: wrap;');
Imagine needing to adjust this style. It'd be easy to find and modify in a CSS file organized by its class, but baked into JS as a string risks burying it in the codebase. Try to limit your JS to adding, removing and toggling CSS classes and minimize touching element.style.
Separation of structure (HTML), presentation (CSS) and behavior (JS) makes front-end codebases easier to maintain. For larger apps, use a component-based system as most SPA frameworks and libraries do these days. If you do use CSS-in-JS, use a template literal string and keep it to one CSS property per line.
Watch out for floating-point weirdness
The code below stores a two-way bound property in the DOM, then re-parses it back into memory per call:
function addOpacity(element) {
let numElement = +(element.style.opacity);
if (numElement === 1){
return numElement;
}
numElement += 0.10;
return numElement;
}
Concerning is numElement === 1 after incrementing += 0.1 per click. It's probably fine in JS, but in other languages, comparing 1.0 and 1.0 is often not going to work. Safer to use >= 1 or an epsilon comparison.
addOpacity also in moves half of its logic to the caller:
element[0].style.opacity = `${addOpacity(element[0])}`;
Why not complete the abstraction and do that assignment in the addOpacity function? Also, the template literal is unnecessary--the string conversion is already done under the hood.
Similarly, addColor has a confusing contract. It really adds an event listener that adds color only later when and if the user mouses over rather than immediately as the name suggests. I'd call it changeColorOnMouseover or something like that.
Don't query the DOM unnecessarily
Also, the addColor mouseover listener re-queries all elements with a class name when there's supposedly only one. document.querySelector would probably be easier: return the one and only, skipping the [0]. Or drop the query entirely and use event.target which is the hovered element.
Even better, just use the element you're adding the listener to!
Create the right abstractions
Your criteria for when to break out logic seems arbitrary. For example, why was getInput broken out into its own function, but not the code to produce a random color, #${(Math.floor(Math.random()*16777215).toString(16))}? The former is a one-liner with no general value that's only called from a single place, whereas the latter is ugly to look at in-line and could easily be a general-purpose library function in an npm package.
Be careful of creating functions without good reason to. When you do, it should be to clean up the logic in the caller by breaking out sub-steps, to enable code reuse and maintainability and to generalize repeated code.
Suggested rewrite
I left prompt in and kept reading opacity from the DOM, but if the app were any larger I'd consider creating objects for each cell to keep track of opacity and other properties in memory, then reflect these properties to the DOM in one direction as described above.
"use strict";
const randomHexColor = () =>
`#${Math.floor(Math.random() * 16777215).toString(16)}`;
const removeAllChildNodes = parent => {
while (parent.firstChild) {
parent.removeChild(parent.firstChild);
}
};
const increaseOpacity = (el, by = 0.1) => {
el.style.opacity = Math.min(1, +el.style.opacity + by);
};
const makeGridCell = size => {
const div = document.createElement("div");
div.style.width = div.style.height = `${size}px`;
div.classList.add("grid-cell");
return div;
};
const addGridMouseoverListener = () => {
const grid = document.querySelector(".grid-container");
grid.addEventListener("mouseover", event => {
const cell = event.target.closest("div");
if (
cell &&
cell.classList.contains("grid-cell") &&
grid.contains(cell)
) {
increaseOpacity(cell);
cell.style.background = randomHexColor();
}
});
};
const addNewGridButtonListener = () => {
const grid = document.querySelector(".grid-container");
const button = document.querySelector(".new-grid");
button.addEventListener("click", () => {
removeAllChildNodes(grid);
const message = "Enter a whole number between 1 and 100";
const sideLen = +prompt(message);
const w = parseInt(getComputedStyle(grid).width);
for (let i = 0; i < sideLen * sideLen; i++) {
grid.appendChild(makeGridCell(w / sideLen));
}
});
};
addGridMouseoverListener();
addNewGridButtonListener();
.grid-container {
display: flex;
flex: 1 1 0;
flex-wrap: wrap;
height: 960px;
width: 960px;
margin: auto;
border: solid black 1px;
}
.grid-cell {
box-sizing: border-box;
border: 1px solid black;
}
<button class="new-grid">Generate New Grid</button>
<div class="grid-container"></div>
consteverywhere out of habit, unless you'll ever change the variable, in which case uselet. E.g.const promptInput,for(let i =etc. You seem to have a globaluniqueClass. Seems like you're tracking how many instances of something are being created. Why is that necessary? \$\endgroup\$