Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added GameOver(Restart button) and score board. #89

Conversation

mukesh14149
Copy link
Contributor

Fixes issue #69
Here is the preview of starting the game.
screenshot from 2016-02-12 16 21 26

Here is the preview when a character hit a obstacle.
screenshot from 2016-02-12 16 21 37

After click on restart button it will come back to initial starting screen and right now score board does not have any functionality to do, it is just a layer.

@niccokunzmann
Copy link
Member

We can close #85 in favor of this, right?

@@ -69,7 +69,23 @@ Flappy.prototype = {

//On collision code here.
if (this._isCollided)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an isCollided method which you should use in favor of the attribute.

@niccokunzmann
Copy link
Member

It looks good! I added some notes on formalities.

document.getElementById('layer7').style.display="none";
document.getElementById('layer13').style.display="none";
document.getElementById('layer14').style.display="none";
document.getElementById('layer15').style.display="none";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the style we should use.
Have a look at https://github.com/fossasia/flappy-svg/blob/gh-pages/javascript/background.js#L80
Which layers are you hiding?

@mukesh14149
Copy link
Contributor Author

@niccokunzmann I updated the code check it now.


function onCollision(){
document.getElementById('layer17').style.display="inline";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please do not use the id. Use the name. it is more descriptive and does not change when editing with Inkscape. You can use these functions:
https://github.com/fossasia/flappy-svg/blob/gh-pages/javascript/layers.js#L60
Also I still do not know from the code what this layer17 is.

@mukesh14149
Copy link
Contributor Author

@niccokunzmann

function stopAllBackgrounds(){

var keys = Object.keys(backgrounds);
for (i = 0; i < keys.length; i++){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use backgrounds.length

@mukesh14149
Copy link
Contributor Author

@niccokunzmann is that fine now?

@niccokunzmann
Copy link
Member

no. two small things:

  • use flappy.stopFlapping() - This a an object oriented design decision: objects should be asked to do something. Just stopping the background will achieve the same result for the user but not for the programmer. It is like messing with the objects internals without asking it. You should ask flappy to stop itself instead
  • use backgrounds.length

Then I would like to merge it. I understand I am criticizing a lot. Every line you do not write to achieve the goal is a line less that other people need to understand.

@mukesh14149
Copy link
Contributor Author

@niccokunzmann

for (i = 0; i < backgrounds.length; i++){
stopMovingBackgound(keys[i]);
}
stopMovingBackgound(flappy.stopFlapping());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the return value of stopFlapping()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry its my mistake its only be flappy.stopFlapping() instead of stopMovingBackgound(flappy.stopFlapping());

@niccokunzmann
Copy link
Member

@mukesh14149 I commented on a line. If you commit, please take the time to write down what the commit means in the message.

@mukesh14149
Copy link
Contributor Author

Ok @niccokunzmann i will take care from next time.

@niccokunzmann
Copy link
Member

@mukesh14149 It looks really good!

niccokunzmann added a commit that referenced this pull request Feb 16, 2016
…lision

Added GameOver(Restart button) and score board.
@niccokunzmann niccokunzmann merged commit e595a5d into fossasia:gh-pages Feb 16, 2016
@niccokunzmann niccokunzmann mentioned this pull request Feb 16, 2016
@hkveeranki hkveeranki mentioned this pull request Jul 31, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants