Change Models, new Game Button and Score #4

ghost · 2017-06-19T23:13:25Z

Change Models so it looks more like the Tetris everyone knows.
Added a new Game button + Score on the top.

Change rotation so it looks more like the Tetris everyone knows. Added a new Game button + Score on the top.

Press P to pause Game

dionyziz

Thank you for taking the time to add these features. These are not bad changes and I would like to see them merged. Please check my comments in the code.

dionyziz · 2017-06-23T20:30:41Z

index.html

+
+
+
+    <center>


Use CSS to center

dionyziz · 2017-06-23T20:30:51Z

README.md

@@ -1,5 +1,7 @@
 A very simple HTML5 version of Tetris, for educational purposes, made in 45 minutes.

+//Added Score, Pause, new Game and changed the rotation of shapes
+


I don't think we need this comment in the README

dionyziz · 2017-06-23T20:31:58Z

js/tetris.js

+var pause = false;
+
+var button = document.getElementById("newgame");
+button.addEventListener ("click", newGame);


Please keep code formatting according to the rest of the codebase: Single quotes, space after ,, no space before (, and so on

dionyziz · 2017-06-23T20:32:14Z

js/tetris.js

+     0,1,0,0,
+     1,1,1,0]
+
+


Not sure why the 0's are necessary here?

For centering the objects and understanding

dionyziz · 2017-06-23T20:32:40Z

js/tetris.js

-            newGame();
-            return false;
+
+    if(!pause) {


Better use an early fail condition with return to avoid indentation level:

if (pause) { return; }

dionyziz · 2017-06-23T20:36:18Z

js/tetris.js

+var score = 0;
+var pause = false;
+
+var button = document.getElementById("newgame");


This should be part of the rendering file.

dionyziz · 2017-06-23T20:36:35Z

js/tetris.js

@@ -175,6 +213,8 @@ function valid( offsetX, offsetY, newCurrent ) {
 }

 function newGame() {
+    score = 0;
+    document.getElementById("score").innerHTML = "Score: " + score.toString();


Move to renderer

Not sure how to move this to renderer without creating a reference anyway, since renderer doesn´t get information about a new Game or ?

Yes, you need to create a reference. The score variable will need to live in tetris.js, but the HTML update will need to be in render.js. This makes for better architecture and modularity.

dionyziz · 2017-06-23T20:36:48Z

style.css

@@ -1,5 +1,16 @@
 canvas {
    display: block;
    margin: auto;
-    border: 1px solid black;
+    border: 1px solid black;}


Mind your coding style

dionyziz · 2017-06-23T20:37:01Z

style.css

+    border: 1px solid black;}
+
+div.container p {
+    font-family: Arial;


Good to include a generic fallback font; sans-serif?

dionyziz · 2017-06-23T20:37:22Z

style.css

}
+

Since you're making CSS changes, a screenshot will assist in the code review. Could you please provide one?

dionyziz · 2017-06-23T22:53:41Z

I did not make any changes in the code – just left comments of recommended changes for you to make before I can merge. Please make these changes and I will review your code again. Once you make the changes, you can push to your branch so that they are reflected in this Pull Request without opening a new one.

ghost · 2017-07-06T11:43:12Z

Cleaned up the code except the score reset.

dionyziz · 2017-08-21T23:02:36Z

js/render.js

+var score = 0;
+var pause = false;
+var button = document.getElementById( "newgame" );
+button.addEventListener( "click", newGame );


Use single quotes to be consistent with the rest of the coding style.

dionyziz · 2017-08-21T23:02:56Z

js/tetris.js

-        ++currentY;
+
+    if(pause) {
+        return;
    }


Please be consistent in your indentation method

dionyziz · 2017-08-21T23:03:03Z

js/tetris.js

-    if ( valid( 0, 1 ) ) {
-        ++currentY;
+
+    if(pause) {


Use a space after if

dionyziz · 2017-08-21T23:03:39Z

js/tetris.js

+            newCurrent[y] = [];
+            for (var x = 0; x < 4; ++x) {
+                newCurrent[y][x] = current[3 - x][y];
+            }
        }


Not sure why you are making these changes? If you want to change the existing coding style into something else, please do so in a separate PR and do it across the whole project, not only incidentally.

dionyziz · 2017-08-21T23:03:50Z

js/tetris.js

@@ -109,6 +129,9 @@ function clearLines() {
            }
        }
        if ( rowFilled ) {
+
+            score = score + 1;
+            document.getElementById("score").innerHTML = "Score: " + score.toString();


use single quotes

dionyziz · 2017-08-21T23:03:58Z

js/tetris.js

    return newCurrent;
 }

 // check if any lines are filled and clear them
 function clearLines() {
+


Do not begin a function with an empty line

dionyziz · 2017-08-21T23:04:43Z

js/tetris.js

                ++currentY;
            }
            break;
        case 'rotate':
            var rotated = rotate( current );
-            if ( valid( 0, 0, rotated ) ) {
+            if ( valid( 0, 0, rotated ) && !pause ) {


This code can be made simpler by moving the check for a pause key to the beginning of the function before the switch is ran. Then you won't need to repeat yourself in each case, as you can return early.

dionyziz · 2017-08-21T23:05:25Z

js/tetris.js

@@ -175,6 +213,8 @@ function valid( offsetX, offsetY, newCurrent ) {
 }

 function newGame() {
+    score = 0;
+    document.getElementById("score").innerHTML = "Score: " + score.toString();


Yes, you need to create a reference. The score variable will need to live in tetris.js, but the HTML update will need to be in render.js. This makes for better architecture and modularity.

dionyziz · 2017-08-21T23:05:34Z

style.css

+    text-align: center;
+}
+
+Button {


lower-case button

dionyziz · 2017-08-21T23:05:38Z

style.css

@@ -3,3 +3,19 @@ canvas {
    margin: auto;
    border: 1px solid black;
 }
+
+div{


space after div

dionyziz · 2017-08-21T23:06:30Z

This is actually looking nice and I'm looking forward to merging it – but I have requested some more changes mostly pertaining to style. Thanks for your contribution!

dionyziz · 2018-06-07T12:42:01Z

Are you interested in finishing these changes? I'd be very happy to merge them once you take care of the small issues I mentioned.

Change rotation, new Game Button and Score

689fbcb

Change rotation so it looks more like the Tetris everyone knows. Added a new Game button + Score on the top.

ghost changed the title ~~Change rotation, new Game Button and Score~~ Change Models, new Game Button and Score Jun 19, 2017

JohannesKnecht added 2 commits June 20, 2017 18:40

Adding Pause

87a3009

Press P to pause Game

Update Readme

87d9572

dionyziz requested changes Jun 23, 2017

View reviewed changes

Cleaning up Code

a2dc622

JohannesKnecht added 3 commits July 6, 2017 13:44

Code Cleanup

50dd9c3

Code Cleanup

819f945

Code Cleanup

96e1583

dionyziz requested changes Aug 21, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Models, new Game Button and Score #4

Change Models, new Game Button and Score #4

ghost commented Jun 19, 2017 •

edited by ghost

Loading

dionyziz left a comment

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

ghost Jul 6, 2017 •

edited by ghost

Loading

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

ghost Jul 6, 2017 •

edited by ghost

Loading

dionyziz Aug 21, 2017

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

dionyziz Jun 23, 2017

ghost Jun 23, 2017

dionyziz commented Jun 23, 2017

ghost commented Jul 6, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz Aug 21, 2017

dionyziz commented Aug 21, 2017

dionyziz commented Jun 7, 2018

		@@ -1,5 +1,7 @@
		A very simple HTML5 version of Tetris, for educational purposes, made in 45 minutes.

		//Added Score, Pause, new Game and changed the rotation of shapes

Change Models, new Game Button and Score #4

Are you sure you want to change the base?

Change Models, new Game Button and Score #4

Conversation

ghost commented Jun 19, 2017 • edited by ghost Loading

dionyziz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 6, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Jul 6, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dionyziz commented Jun 23, 2017

ghost commented Jul 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dionyziz commented Aug 21, 2017

dionyziz commented Jun 7, 2018

ghost commented Jun 19, 2017 •

edited by ghost

Loading

ghost Jul 6, 2017 •

edited by ghost

Loading

ghost Jul 6, 2017 •

edited by ghost

Loading