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 restartButton to main screen #69

Merged
merged 5 commits into from
Nov 22, 2017

Conversation

govindbalaji-s
Copy link
Contributor

Fixes Issue #68
In JS_game.html:
I have added the button for restartgame in buttonWrapper div.
It's onClick is restartGame() .

In Game.js, I have added restartGame() function which stops gameArea and calls startGame(). I have also made space bar to work anytime. Instead of refreshing the page, it now calls restartGame()

In JS_game.html:
I have added the button for restartgame in buttonWrapper div
It's onClick is restartGame() .
git status
In Game.js, I have added restartGame() function which stops gameArea and calls startGame(). I have also made space bar to work anytime. Instead of refreshing the page, it now calls restartGame()
@govindbalaji-s
Copy link
Contributor Author

Except those changes there aren't changes anywhere else. I think the changes includes so much extra lines as changed because of tab vs space inconsistency with my editor?

@EdwardDunn
Copy link
Owner

@govindbalaji-s Sorry about the delay looking at your PR, thanks for your contribution. The restart button works fine but could you position it lower on the screen so that it is separated from the control buttons.

@govindbalaji-s
Copy link
Contributor Author

Will it be good if the restart button is placed next to the level message at the top?

@EdwardDunn
Copy link
Owner

@govindbalaji-s My thought was that it would be best on a new line below the on screen controls. If you want to try it next to the level message we can see what it looks like.

@govindbalaji-s
Copy link
Contributor Author

Sorry was caught up in the school,
Screenshot
The problem is the game can't be played without zooming out. I will now make the new commits soon.

I have also made restart button in gameovermodal call the restartButton function
instead of history.go(), which i missed to do in last commit. I tried to rescale the canvas and add the restart button at the below. Unfortunately that was not easy since raw constants were used everywhere and that somehow disabled the jump control. So I had to add this button near level display. I tried to shrink the restart button to 55px but thats not working, due to image size(I think?).
@EdwardDunn
Copy link
Owner

@govindbalaji-s The restart button looks good at the top. Can you adjust your CSS, when the window is re-sized the button does not stay fixed. Looks good though!

I have made restart button's position vary with the resizing of windows.
Along with that the sizes of all buttons are now made to vary.
@govindbalaji-s
Copy link
Contributor Author

I am sorry for the delay. Check this out now.

@EdwardDunn
Copy link
Owner

@govindbalaji-s Thanks for sorting the CSS issue with the restart button. The changes to the control button sizes make it difficult when played on a mobile device. When the screen is shrinked the buttons become very small and difficult to touch. This is the reason their size was fixed.

The restart button isn't as important for game play, and obviously if it is on the game screen it does need to shrink with the screen size.

Can you set the control buttons sizes back to fixed.

Sorry if this seems trivial but its important to make sure its playable on all devices. Otherwise your work is awesome!

The resizing width of other buttons have been moved now to be applied only on the inside-canvas-restart-button.
@govindbalaji-s
Copy link
Contributor Author

I am a newbie so I am slow sorry. I have fixed that and now only restart button resizes. the buttonImages class looks a bit unnecessary but I haven't removed them for now incase in future if any css for all button images is needed. If you think they can be removed now, I will do that.

@EdwardDunn
Copy link
Owner

@govindbalaji-s Thanks, works great now! Can you pull down the recent changes to your branch and then push again, i cant merge your branch as it is out of date.

@EdwardDunn EdwardDunn merged commit 0b964c1 into EdwardDunn:master Nov 22, 2017
@govindbalaji-s
Copy link
Contributor Author

Thank you for being patient throughout and guiding me.

@EdwardDunn
Copy link
Owner

@govindbalaji-s No problem, thanks for your contribution!

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