Naming.
Use a consistent and idiomatic naming style. Most important is consistency within the code base.
JS
For JS use cammelCase for all names appart from object that are instanciated with the new
token, in that case use PascalCase. For constants you can use UPPER_CASE_SNAKE but as objects and arrays are often declared as constants you can end up with a lot of UPPER_CASE_SNAKE in the code, which does not read well.
There seams to be no general agreement as to what style to use. I personally only use UPPER_CASE_SNAKE for constants that are basic types (Number, Bool, String, etc..) and for object properties that are not writable.
CSS
Currently your CSS naming is inconsistent. This will make it harder to write good code as you will be spending mental energy remembering what style you used for what name, eg you have the two classes named button_slow_remove
and slow-fade
you remember the name not the hyphen or underscore, later you come back to the project and you will forever be checking "Was that name underscore or hyphen"
For CSS you can use Block Element Modifier (BEM) naming convention. The link provided a good introduction with plenty of resources to get you started.
Note BEM is but one of many and there is nothing wrong with defining your own, as long as you are consistent.
Plurals
Names give hints to a variables content. We pluralize arrays, lists, sets, etc (sometimes even strings if we are using them as an array of characters), and use the singular for single items.
You are using let add_btns = document.querySelector('.button_add');
and someone, even you coming back later will automatically think buttons (an array / array like), its a buton, use the singular const addBtn = document.querySelector('.button_add');
Also it is ok to use common abbreviations, btn
is just as good a name as button
Always BE CONSISTENT!
Code Style
- NEVER start a line with a ( unless it is part of a multi line statement or expression. You have
(function () {
If someone adds a line above this and is one of those crazzy people that say they don't need to terminate lines
Eg
var a = 0
var b = a // << this line is exterminated
(function() { // << will parse as var b = a(function() { and throw an error
So play it safe and add the ;
for those that don't.
var a = 0
var b = a // << this line is un-terminated
;(function() { // the ; terminates any multi line statement or expressions
// while at you can use the shorter form
;(()=>{
...code
})()
Use arrow functions to reduce code noise. Code noise is any code that is not needed.
Always declare variables as const
, only use var
or let
if the variable needs to change. The following variables are all incorrectly defined as let
yet don't change buttons, add_btns, btn, id, clear_btns, slow_remove_btns
Take care with the indenting.
Reduce code noise. A measure code quality can be defect density, this value is linked to the coder and their experience level. Your defect density will remain consistent (though time will reduce the value), it means more code, more bugs. The easiest way to reduce the number of bugs in your code is to reduce the code size. You have many unneeded lines of code, many due to style.
You have
btn.addEventListener("click", function () {
let id = this.id;
alert("my id is: " + id);
});
// less noise
btn.addEventListener("click", (e) => alert("My id is: " + e.target.id));
// or to keep `this`
btn.addEventListener("click", function() { alert("My id is: " + this.id) });
API use
Don't use setAtribute
if its not needed. The general rule is setAtribute
is only needed for properties that are not part of the DOM's API and needs to be reflected in the markup.
When setting text for an element use the textContent
property, ONLY use innerHTML
if you are adding HTML
Use an element's id
to select it rather than the class name. Using class name forces you to create CSS rules for elements with the same style.
Rather than add the buttons directly to the DOM, which will force a re-flow and slow things down, use a documentFragment
, or create an off page container to work with, then when complete add them to the page in one call / one re-flow.
Bugs
You have some bugs in the code.
- Your code comments say 40 buttons yet the code generates 41 buttons.
The code at fault is
const BUTTON_NUM = 40;
// ... some code removed
let i = 0;
while (i <= BUTTON_NUM) { // from 0 to 40 are 41 steps
// Should be
while (i < BUTTON_NUM) {
- You are adding id's too often. Inside the while loop you do a for in loop over the buttons you have already created. This means the first button is assigned the same values 41 times.
Testing
You don't write your code for testing, you test code. You have not presented any testing code so there is nothing to review.
Some other points
Keep things simple. You are storing an array of buttons, you are adding events to each fade button. There is no need. Add a single event to the button container, use the event.target argument to get the element clicked on. Don't store elements if you don't need them
Magic numbers and strings are bad. You code has many strings referring to class names all over the place. If you need to make changes, it requires you to make changes in many places, which is a pain and potencial defect.
You started well with const CLS = ["thin", "button"]
and const BUTTON_NUM = 40
but then started mixing in string as needed. Keep all magic values (constants that are subject to design change) in one place, that need only one change to the source.
From the Comments
Try to make it class – zb
NO Don't! JS does not have classes, only objects. class
is a syntax used to create objects.
You don't need to create an object, you have correctly encapsulated your code with a singleton, it needs no exposed API (object with properties and functions), and works as a static single instance (It does not need many copies).
OO coding is a complex subject, and most low level coders do not have a good grasp of when and how to use it effectively. Making it a class does not mean it makes it better code.
Rewrite
From the ground up and using an object btns
to hold button related consts, and functionality.
To improve portability, or really just make it more flexible I have create functions to cover the common tasks you code was doing. Eg query
, addEvent
, clickEvent...
I have also deliberately referenced element buttonContainer
to show that it is a legitimate way to access elements. But do note that that name is global and not isolated to the singleton.
The layout and CSS have been improvised as you only provided CSS and that had unrelated content.
The code adds 3 events for each of the main ui buttons. The add button adds the buttons to a fragment and then to the page (clearing the previous content beforehand)
Then a final event is added to the button container to manage the alert showing button id.
It is designed to simplify the adding of Remove FX. You need only change 3 lines of JS and that is only data to add a new fade FX. (of course you need to add the HTML and CSS)
;(() => {
const btns = {
count : 13,
className : "buttons buttons--style-default buttons--action-fader",
text : "Button #", // the # is replaced with the ID
id : "button#", // the # is replaced with the ID
actions : {add : "#addBtn", slowFade : "#slowFadeBtn",clear : "#clearBtn" },
anim : { slowFade : "buttons--anim-fade", clear : "buttons--anim-clear" },
animNames : ["slowFade", "clear"],
reset() {
btnContainer.innerHTML = "";
for (const name of btns.animNames) { btnContainer.classList.remove(btns.anim[name]) }
},
remove(method) { btnContainer.classList.add(method) },
};
const query = query => document.querySelector(query);
const addEvent = (el, func, type = "click") => el.addEventListener(type, func);
const clickEvent = event => alert("My id is : " + event.target.id);
const actionEvent = name => addEvent(query(btns.actions[name]), () => btns.remove(btns.anim[name]));
const frag = document.createDocumentFragment();
addEvent(query(btns.actions.add), () => {
btns.reset();
for (let i = 0; i < btns.count; i ++) {
frag.appendChild( Object.assign( document.createElement("button"), {
id : btns.text.replace("#", i),
className : btns.className,
textContent : btns.text.replace("#", i),
})
);
}
btnContainer.appendChild(frag);
});
addEvent(btnContainer, clickEvent);
btns.animNames.forEach(actionEvent);
}) ();
.buttons {
background: transparent;
align-self: center;
}
.buttons--style-default {
padding: 0.5rem 0.5rem;
transition: all .5s ease;
outline: none;
color: #41403E;
cursor: pointer;
font-size: 1rem;
box-shadow: 20px 38px 34px -26px rgba(0, 0, 0, 0.2);
border-radius: 255px 15px 225px 15px/15px 225px 15px 255px;
}
.buttons--action-ui {
align-self: center;
margin: 0 0.2rem;
}
.buttons--action-fader {
display: inline-flex;
margin: .2rem 0.2rem;
border: solid 2px #41403E;
}
.buttons--style-default:hover {
box-shadow: 2px 8px 4px -6px rgba(0, 0, 0, 0.3);
}
/* Animations or animation like rules for buttons */
.buttons--anim-clear {
display: none!important;
}
.buttons--anim-fade {
opacity: 0;
transition: opacity 4s linear;
}
<div class="buttons">
<button id="addBtn" class="buttons buttons--style-default buttons--action-ui" >Add buttons</button>
<button id="clearBtn" class="buttons buttons--style-default buttons--action-ui" >Remove buttons</button>
<button id="slowFadeBtn" class="buttons buttons--style-default buttons--action-ui" >Remove with fade</button>
<div id="btnContainer" class = "buttons buttons-container">
</div>
</div>
slow-fade
andclear
classes and turning the code into a live snippet. \$\endgroup\$