Loading items for an eCommerce site

Question

I am attempting to make an eCommerce site from scratch. Right now I have the following code that gets all the items from a Firebase Firestore database and then adds HTML into an items div element based on the item's info.

var items = [];
var itemsDiv = document.getElementsByClassName("items")[0];
db.collection("items").get().then((querySnapshot) => {
 querySnapshot.forEach((doc) => {
 var docData = doc.data();
 items.push(docData);
 });
 renderItems();
});
function renderItems() {
 console.log(items);
 items.forEach(function(item) {
 itemsDiv.innerHTML += `
 <div class="item">
 <img class="itemImgBackground" src="assets/${item.name.replace(" ", "")}.png">
 <img class="itemImg" src="assets/${item.name.replace(" ", "")}.png">
 <span class="itemName">${item.name}</span>
 <span class="itemCondition">${item.condition}</span>
 <span class="itemPrice">${item.price}</span>
 </div>
 `
 });
}

What can I do to make my code better?

Answer 1

Here are a few things:

• good you're using ` (template literals) to format your HTML inside the JavaScript code

• don't use innerHTML += as it forces the browser to re-render the DOM when called. Read more here: Why is Element.innerHTML += bad code?. Instead create an HTML element with .createElement and append it to an existing DOM element .appendChild.

  So instead of appending the the HTML string <div class="item">...</div> to the parent div you can:

  1. create a wrapping div element:

     const div = document.createElement('div');
     

  2. append the HTML string to the div wrapper (remember div is an object of type Element but it's not already rendered in the page DOM). You can use all of Element's methods on it, including .innerHTML:

     div.innerHTML = `
      <div class="item">
      ...
      </div>
     `;
     

  3. then append the wrapper's first child to the parent element:

     divParent.appendChild(div);
     

• you can use .querySelectorAll instead of .getELementByClassName. The former allows you to pass in an actual CSS selector instead of a className (the latter method). In your case you only want the first matching element, so use .querySelector instead.

• I always use single quotes ' for JavaScript strings and double quotes " when I need quotes inside the strings e.g. with HTML attributes. That's personal preference though.

• no need to define the variable docData as doc.data() is already pretty self-explanatory

• you can use arrow functions outside also (arg1, arg2, ...) => {} or if you have a single argument arg => {} (no need for the parenthesis).

• you could use const for constant variable to make sure they don't get their values changed: for instance itemsDiv (fyi: renamed to itemsParent for clarity) can be defined with const instead of var.

---

Here's a possible refactoring of your code:

const itemsParent = document.querySelector('.items')[0];
let items = [];
const renderItems = items => {
 items.forEach(item => {
 const div = document.createElement('div');
 div.innerHTML = `
 <div class="item">
 <img class="itemImgBackground" src="assets/${item.name.replace(" ", "")}.png">
 <img class="itemImg" src="assets/${item.name.replace(" ", "")}.png">
 <span class="itemName">${item.name}</span>
 <span class="itemCondition">${item.condition}</span>
 <span class="itemPrice">${item.price}</span>
 </div>
 `.trim();
 itemsParent.appendChild(div.firstChild);
 });
}
db.collection('items').get().then((querySnapshot) => {
 querySnapshot.forEach(doc => {
 items.push(doc.data());
 });
 renderItems(items);
});

---

Edit: testing div creation, it appears you need to trim the string first:

const div = document.createElement('div');
div.innerHTML = `
 <div class="parent">
 <div class="child"></div>
 </div>
`.trim();
console.log(div.outerHTML)
console.log(div.firstChild.outerHTML)

• Jordan Baron: The code doesn't work, it happens when you append div.firstChild
• Jordan Baron: For some reason, div.firstChild returns an object. Removing <div class="item" and adding div.className = 'item' after creating the div fixes the problem.
• Ivan: @JordanBaron I have corrected my answer: you first need to trim the string otherwise it won't work. Try again my code snippet to see if it works.
• Sᴀᴍ Onᴇᴌᴀ: "you can use .querySelectorAll instead of .getELementByClassName" but the latter is more than twice as fast...

Answer 2

Similar to what I mentioned in my review of your other Post: Carousel in Vanilla JavaScript, if there is only one element that has class items then it would be more appropriate to use an id attribute and query the DOM using document.getElementById().

---

As Ivan mentioned, when defining an arrow functions: "Parentheses are optional when there's only one parameter name"¹

so instead of:

 querySnapshot.forEach((doc) => {

The parentheses around doc can be removed:

querySnapshot.forEach(doc => {

---

The callback function that takes each element from the querySnapshot pushes the result from calling .data() into items.

querySnapshot.forEach((doc) => {
 var docData = doc.data();
 items.push(docData);
});

This can be simplified using the Array.map() method:

items = querySnapshot.map(doc => doc.data());

If you do that, you could declare items as a const and pass it as an argument to renderItems() instead of declaring it at the top of the script.