Return to Revisions

2 of 7

added 711 characters in body

edited Sep 13, 2019 at 15:53

29.6k
16
45
203

General feedback

Overall this code isn’t bad but could be simplified quite a bit The biggest thing I noticed is that jQuery is used for a few things but could be used a lot more - e.g. selecting DOM elements and manipulating them, adding event handlers, etc. There are some who believe jQuery isn't needed (see sites like youmightnotneedjquery.com.

If you really want to clean things up, run your code through a linter like eslint, jshint, etc. One of the first warnings will likely be to include "use strict" at the top of the JavaScript.

Targeted feedback

JS

functions doing more than necessary? I see the function CreateAddToCartEventListener() calls updateCartCount(). Is that really necessary when adding an event listener?
scoping and DOM ready wrap JS in a DOM ready callback (I.e. $(function() {}) - formerly .ready()) or else an IIFE to limit scope of variables
utilize jquery more As mentioned above, it can be used a lot more - e.g. instead of:
```
 $clearAll = document.getElementById("clearAll");
```
Just select it with the jquery function:
```
 $clearAll = $("#clearAll");
```

Note that such a variable would then contain a jQuery collection and not a single DOM reference anymore, so with that one can call jquery methods on the collection like .hide() and .show()

$clearAll.style.display = "none";

Can be changed to:

 $clearAll.hide();

Also I see things like this:

const CreateAddToCartEventListener = () => {

 var addToCart = document.getElementsByClassName("AddToCart");
 updateCartCount();
 Array.prototype.forEach.call(addToCart, function(element) {
 element.addEventListener("click", function() {

Why not register event handlers like the others:

 $(document.body).on("click", ".AddToCart", function() {

or event simpler with the .click() shortcut method:

 $(document).click('.AddToCart', function() {

global variables - any variable not declared with var (inside a function), let or const e.g. $shoppingCartContainer will be stored as a global variable - on window. In a small application that likely wouldn’t be an issue but it is a good habit to avoid those - especially when you get into a larger application and the same name is used to hold different data.
default to using const for variables that don’t need to be reassigned - e.g. $quantity and then switch to let if it is determined that reassignment is necessary.
setting length of new array to zero - when would the length of a new array be something other than zero?
```
var cart = [];
```
```
 cart.length = 0;
```
use spread operator - since ecmascript-6 features like arrow operators are used then instead of calling:
```
Array.prototype.forEach.call(addToCart, function(element) {
```
The collection can be put into an array with the spread operator:
```
 [...addToCart].forEach( function(element) {
```
I see this line
```
window.setTimeout(function() {}, 1000); // prevent flickering
```
but I am not convinced that calling an empty function after 1 second will prevent flickering. How did you reach that conclusion?

HTML

The <script> tags are outside the <html> tags. They are typically placed in the <head> or at the end of the <body> tag. For more information on this, refer to this post.

CSS

multiple max-width media queries with same style I see the following rulesets:

@media (max-width: 1333px) {

 .column {
 flex-basis: 33.33%;
 }
 }

@media (max-width: 1073px) {

 .column {
 flex-basis: 33.33%;
 }
 }

The second ruleset is overriding the first, but it doesn't appear to do anything different.

excess margin style: #totalCartItems has margin with top, right, bottom and left, but then there is a separate style for margin-left: auto. Why not move that auto into the first margin style if it is needed?

answered Sep 13, 2019 at 15:36

Sᴀᴍ Onᴇᴌᴀ ♦

29.6k
16
45
203

default