Skip to main content
added 553 characters in body
Source Link
ComFreek
  • 771
  • 8
  • 20

Drawing comments

  • Currently, one can also draw with the right mouse button, however, upon releasing it, the context menu is still triggered. Contrary to generally disabling the right mouse button, which is abysmal UX, it is indeed useful for a gaming/drawing <canvas>.
    It works with the following code (live demo):

      board.addEventListener("contextmenu", function(e) {
        e.stopPropagation();
        e.preventDefault();
        return false;
      });
    

Code comments

  • It's good that you use === and !== for (in)equality checks!

  • You can use Object.assign instead of your custom extend() function: opts = Object.assign({}, defaults, options || {}). Be aware, though, that it cannot deep-copy/merge!

  • I got into the habit of declaring every variable either with let or const and never with var. In 99% of the cases, you really want block-scoped and not function-scoped variables.

    Especially, I would recommend against declaring your helper functions as var getMousePos = function(evt) {. This way, the variables could even be reassigned, which is almost always a strange behavior for functions.
    Either write const getMouePos = function(evt) { or function getMouePos(evt).

  • Use consistent string quotes: either '...' or "...", but not both if not required (e.g. const str = "Eve's dog is barking").

  • The code repeatedly makes unnecessary function calls: draw(getTouchPos(e).x, getTouchPos(e).y). Granted, they are probably super cheap in your case, but could be avoided even without doind any kind of premature optimization:

  • Make draw accept a {x, y} object. If you now also replace startX, startY by a single variable, this would make the internal API much more consistent.

  • Introduce a variable:

         // Previously
         draw(getTouchPos(e).x, getTouchPos(e).y);
    
         // Now
         const touchPos = getTouchPos(e);
         draw(touchPos.x, touchPos.y);
    
         // Previously
         startX = getTouchPos(e).x;
         startY = getTouchPos(e).y;
    
         // Now
         const touchPos = getTouchPos(e);
         [startX, startY] = [touchPos.x, touchPos.y]; // or arguably with two lines
    
  • It's good that you use === and !== for (in)equality checks!

  • You can use Object.assign instead of your custom extend() function: opts = Object.assign({}, defaults, options || {}). Be aware, though, that it cannot deep-copy/merge!

  • I got into the habit of declaring every variable either with let or const and never with var. In 99% of the cases, you really want block-scoped and not function-scoped variables.

    Especially, I would recommend against declaring your helper functions as var getMousePos = function(evt) {. This way, the variables could even be reassigned, which is almost always a strange behavior for functions.
    Either write const getMouePos = function(evt) { or function getMouePos(evt).

  • Use consistent string quotes: either '...' or "...", but not both if not required (e.g. const str = "Eve's dog is barking").

  • The code repeatedly makes unnecessary function calls: draw(getTouchPos(e).x, getTouchPos(e).y). Granted, they are probably super cheap in your case, but could be avoided even without doind any kind of premature optimization:

  • Make draw accept a {x, y} object. If you now also replace startX, startY by a single variable, this would make the internal API much more consistent.

  • Introduce a variable:

         // Previously
         draw(getTouchPos(e).x, getTouchPos(e).y);
    
         // Now
         const touchPos = getTouchPos(e);
         draw(touchPos.x, touchPos.y);
    
         // Previously
         startX = getTouchPos(e).x;
         startY = getTouchPos(e).y;
    
         // Now
         const touchPos = getTouchPos(e);
         [startX, startY] = [touchPos.x, touchPos.y]; // or arguably with two lines
    

Drawing comments

  • Currently, one can also draw with the right mouse button, however, upon releasing it, the context menu is still triggered. Contrary to generally disabling the right mouse button, which is abysmal UX, it is indeed useful for a gaming/drawing <canvas>.
    It works with the following code (live demo):

      board.addEventListener("contextmenu", function(e) {
        e.stopPropagation();
        e.preventDefault();
        return false;
      });
    

Code comments

  • It's good that you use === and !== for (in)equality checks!

  • You can use Object.assign instead of your custom extend() function: opts = Object.assign({}, defaults, options || {}). Be aware, though, that it cannot deep-copy/merge!

  • I got into the habit of declaring every variable either with let or const and never with var. In 99% of the cases, you really want block-scoped and not function-scoped variables.

    Especially, I would recommend against declaring your helper functions as var getMousePos = function(evt) {. This way, the variables could even be reassigned, which is almost always a strange behavior for functions.
    Either write const getMouePos = function(evt) { or function getMouePos(evt).

  • Use consistent string quotes: either '...' or "...", but not both if not required (e.g. const str = "Eve's dog is barking").

  • The code repeatedly makes unnecessary function calls: draw(getTouchPos(e).x, getTouchPos(e).y). Granted, they are probably super cheap in your case, but could be avoided even without doind any kind of premature optimization:

  • Make draw accept a {x, y} object. If you now also replace startX, startY by a single variable, this would make the internal API much more consistent.

  • Introduce a variable:

         // Previously
         draw(getTouchPos(e).x, getTouchPos(e).y);
    
         // Now
         const touchPos = getTouchPos(e);
         draw(touchPos.x, touchPos.y);
    
         // Previously
         startX = getTouchPos(e).x;
         startY = getTouchPos(e).y;
    
         // Now
         const touchPos = getTouchPos(e);
         [startX, startY] = [touchPos.x, touchPos.y]; // or arguably with two lines
    
Source Link
ComFreek
  • 771
  • 8
  • 20

  • It's good that you use === and !== for (in)equality checks!

  • You can use Object.assign instead of your custom extend() function: opts = Object.assign({}, defaults, options || {}). Be aware, though, that it cannot deep-copy/merge!

  • I got into the habit of declaring every variable either with let or const and never with var. In 99% of the cases, you really want block-scoped and not function-scoped variables.

    Especially, I would recommend against declaring your helper functions as var getMousePos = function(evt) {. This way, the variables could even be reassigned, which is almost always a strange behavior for functions.
    Either write const getMouePos = function(evt) { or function getMouePos(evt).

  • Use consistent string quotes: either '...' or "...", but not both if not required (e.g. const str = "Eve's dog is barking").

  • The code repeatedly makes unnecessary function calls: draw(getTouchPos(e).x, getTouchPos(e).y). Granted, they are probably super cheap in your case, but could be avoided even without doind any kind of premature optimization:

  • Make draw accept a {x, y} object. If you now also replace startX, startY by a single variable, this would make the internal API much more consistent.

  • Introduce a variable:

         // Previously
         draw(getTouchPos(e).x, getTouchPos(e).y);
    
         // Now
         const touchPos = getTouchPos(e);
         draw(touchPos.x, touchPos.y);
    
         // Previously
         startX = getTouchPos(e).x;
         startY = getTouchPos(e).y;
    
         // Now
         const touchPos = getTouchPos(e);
         [startX, startY] = [touchPos.x, touchPos.y]; // or arguably with two lines