On right click draw triangle to the canvas

Question 1

This is a simple program that using easelJS adds functionality to the right click, when the right mouse button is clicked a triangle is drawn to the screen. I am just wanting to get the opinions of others on my coding style and how it could be improved.

body{
    margin: 0;
    background-color: #d3d3d3;
    font-family: Sans-Serif;
}

h1{
    font-weight: 100;
    text-align: center;
}

canvas{
    display: block;
    margin: auto;
    background-color: #ffffff;
    border: solid 1px #000000;
}

<!DOCTYPE html>

<html>

    <head>
        <title>Draw Triangle</title>
        <link rel="stylesheet" href="styles.css"/>
        <script src="https://code.createjs.com/easeljs-0.8.2.min.js"></script>
        <script>
            var stage;
            var triangle;

            function init() {
                stage = new createjs.Stage("myCanvas");

                stage.addEventListener("stagemousedown", function(evt) {
                    var e = evt.nativeEvent;
                    var key = e.which || e.keyCode;
                    if(key !== 1){
                        addTriangle(evt.stageX, evt.stageY);
                    }
                });
            }

            function addTriangle(x, y){
                triangle = new createjs.Shape();
                triangle.graphics.beginFill("#000000");
                triangle.graphics.moveTo(x, y).lineTo(x-100, y).lineTo(x-50,y-100).lineTo(x, y);
                stage.addChild(triangle);
                stage.update();
            }
        </script>
    </head>

    <body onload="init();">
        <h1>Right click to draw a Triangle to the canvas</h1>
        <canvas id="myCanvas" width="900" height="700"></canvas>
    </body>

</html>

Expand snippet

Question 2

General problems

It is better to add events via addEventListener rather than in the HTML, this keeps the source code together making it easier to maintain. It also protects your code from 3rd party code overwriting the event handler. If some code later sets window.onload = theirFunction; it overwrites your event that will not get called.
Keep your variables out of the global scope if possible. stage and triangle do not need to be in global scope. triangle is only used in the one function so define it there.
Use const rather than var if you do not intend to modify a variable.
Don’t let lines get too long. 80 character is the usual limit. If a line gets longer than that break it into multiple lines.
Right click will bring up the context menu. You can prevent this by listening to the contextmenu event and calling preventDefault on the event object.
Why the defaulting to keyCode? is that even defined for the easelJS event ?
Don’t add needless code. You define a variable for evt.nativeEvent, and e.which. These are not really needed.
Use correct names for variables. You use key for which button is pressed, this can be confusing when code gets longer.
You say you want to add a triangle on the right click but the code you had will add a triangle for any button except the left button.
Make your code easier to read by using spaces after commas, between operators, after statements and befor { eg if(bar===poo){foo(a,b+2,c);} is better if (bar === poo) { foo(a, b + 2, c); }

The rewrite

Rewrite of your code using the above.

body{
    margin: 0;
    background-color: #d3d3d3;
    font-family: Sans-Serif;
}

h1{
    font-weight: 100;
    text-align: center;
}

canvas{
    display: block;
    margin: auto;
    background-color: #ffffff;
    border: solid 1px #000000;
}

<!DOCTYPE html>

<html>

    <head>
        <title>Draw Triangle</title>
        <link rel="stylesheet" href="styles.css"/>
        <script src="https://code.createjs.com/easeljs-0.8.2.min.js"></script>
        <script>
            addEventListener("load", init);  // add the load event listener 
            function init() {
                // I am getting canvas as I need it to get the contextmenu event
                const canvas = document.getElementById("myCanvas");

                // use const for variables that will not change.
                const stage = new createjs.Stage(canvas);
                stage.addEventListener("stagemousedown", mouseDown);

                // to prevent the context menu popping up
                canvas.addEventListener("contextmenu", (event) => {
                    event.preventDefault();
                });

                // simplify the mouseDown event
                function mouseDown(evt) {
                    if (evt.nativeEvent.which === 3) {
                        addTriangle(evt.stageX, evt.stageY);
                    }
                }

                function addTriangle(x, y) {
                    const triangle = new createjs.Shape(); // define triangle in the scope it is used
                    triangle.graphics.beginFill("#000");
                    triangle.graphics       // break the line to make it more readable and easier to modify
                        .moveTo(x, y)       // use spaces after commas
                        .lineTo(x - 100, y) // and spaces around operators
                        .lineTo(x - 50, y - 100)
                        .lineTo(x, y);
                    stage.addChild(triangle);
                    stage.update();
                }
            }
        </script>
    </head>
    <body>
        <h1>Right click to draw a Triangle to the canvas</h1>
        <canvas id = "myCanvas" width = "900" height = "700"></canvas>
    </body>
</html>

Expand snippet

Additional notes.

I don’t know what your intention is but the triangle seems a little odd not being centered on the mouse click. Also I would define the triangle as a abstract path rather than hard coded to the function.

Is there a reason you put the code in the head of the document. You could have placed it at the bottom of the page and avoided having to wait for the page load event.

Also, maybe you are wanting to learn easelJS, but the load time of easelJS is substantial and what you do can just as easily be done using the native 2D API.

I strongly believe that you should have a very good reason to use a 3rd party library, especially when you are learning. You are best spending your time learning the native (and very good and performant) APIs. You may not always be able to use the library, but you will always have access to the native API

Rewrite without easelJS

To match your code I have implemented a very simple stage. Though what you do does not require a stage object and could be a lot simplier.

Show code snippet

// Please note this code uses a compact style for single line statements
// There is no ; before the closing } to comply with JSLint rule 
// "Semicolan must be followed by newline."

;(() => { // wrapping your code inside a function alows you to keep the global scope clean 
          // the ; befor the ( is to prevent code above it without a ; from 
          // mistaking the ( as a function call

  const canvas = document.getElementById("myCanvas");
  const ctx = canvas.getContext("2d");

  const shapes = { // you could add more shapes here
    triangle: [ [0, -20], [20, 20], [-20, 20] ], // coordinates of triangle
  };

  addEventListener("load", init); // add the load event listener 

  // implement a stage object to hold shapes
  const stage = {
    shapes: [],
    addShape(x, y, color, path) { this.shapes.push({x,  y, color, path }) },
    update() {  // function to redraw all shapes on the stage
      ctx.clearRect(0, 0, canvas.width, canvas.height);
      this.shapes.forEach((shape) => {
        var i;
        ctx.fillStyle = shape.color;
        ctx.beginPath();
        i = 0;
        while (i < shape.path.length) { 
          ctx.lineTo(  // sometimes it is best to break up complex lines
              shape.path[i  ][0] + shape.x,
              shape.path[i++][1] + shape.y
          ) 
        }
        ctx.closePath();  // this is really only needed if you are using stroke
        ctx.fill();
      });
    }
  };

  function init() {
    canvas.addEventListener("mousedown", mouseDown);
    canvas.addEventListener("contextmenu", event => event.preventDefault() );
    function mouseDown(event) {
      const bounds = event.target.getBoundingClientRect();
	  const x = event.pageX - bounds.left - scrollX;
	  const y = event.pageY - bounds.top  - scrollY;
      if (event.which === 3) {
        stage.addShape(x, y, "black", shapes.triangle);
        stage.update();
      }
    }
  }
})();

body {
  margin: 0;
  background-color: #d3d3d3;
  font-family: Sans-Serif;
}

h1 {
  font-weight: 100;
  text-align: center;
}

canvas {
  display: block;
  margin: auto;
  background-color: #ffffff;
  border: solid 1px #000000;
}

<!DOCTYPE html>

<html>

<head>
  <title>Draw Triangle</title>
</head>

<body>
  <h1>Right click to draw a Triangle to the canvas</h1>
  <canvas id="myCanvas" width="900" height="700"></canvas>
  <script>/* you would put your script here */</script>
</body>

</html>

Expand snippet

The simplest way to achieve the same functionality.

Show code snippet

const canvas = document.getElementById("myCanvas");
const ctx = canvas.getContext("2d");
const triangle = [ [0, -20], [20, 20], [-20, 20] ];
canvas.addEventListener("mousedown", mouseDown);
canvas.addEventListener("contextmenu", e => e.preventDefault() );
function mouseDown(event) {
  const bounds = event.target.getBoundingClientRect();
  const x = event.pageX - bounds.left - scrollX;
  const y = event.pageY - bounds.top  - scrollY;
  if (event.which === 3) { drawShape(x, y, triangle) }
}
function drawShape(x, y, shape) {
  var i = 0;
  ctx.fillStyle = "black";
  ctx.beginPath();
  while (i < shape.length) { ctx.lineTo( shape[i][0] + x, shape[i++][1] + y ) }
  ctx.fill();
}

body {
  margin: 0;
  background-color: #d3d3d3;
  font-family: Sans-Serif;
}

h1 {
  font-weight: 100;
  text-align: center;
}

canvas {
  display: block;
  margin: auto;
  background-color: #fff;
  border: solid 1px #000;
}

  <h1>Right click to draw a Triangle to the canvas</h1>
  <canvas id="myCanvas" width="900" height="700"></canvas>

Expand snippet

General problems

The rewrite

Additional notes.

Rewrite without easelJS

Leave a Reply Cancel reply