-
Notifications
You must be signed in to change notification settings - Fork 24
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 a modal allowing the player to choose the level and a start gam… #21
base: main
Are you sure you want to change the base?
Conversation
…e button for starting the game
mention the issue number |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
center the position of modal, add game rules in it. Also note that the timer should not start before the game actually start
style.css
Outdated
padding: 0.5em 1em; | ||
} | ||
|
||
.mn-lt-1 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where is this class used
style.css
Outdated
font-size: 2rem; | ||
} | ||
|
||
.p:not(:only-child) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try to use better names for classes. And if the element is not much significant use some selection operation to target that element instead of assigning a class
style.css
Outdated
.modal.active { | ||
display: grid; | ||
} | ||
#bttn{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use better names
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use better names
Made changes !
style.css
Outdated
*::before, | ||
*::after { | ||
margin: 0; | ||
padding: 0; | ||
box-sizing: border-box; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this part to the top , after the *{ }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move this part to the top , after the *{ }
OK sure !
Issue no.#7 |
Trying to change it ! |
fixed this issue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are some logical mistakes:
- if I do not select the level and close the modal by clicking outside it , it is also starting the game.
- once the game is started the start button is still available and I am able to click on it , and that is causing a glitch in timer
suggestion:
- use the index.js from previous commit, because you have changed quite a lot of thing
- create a function handleLevel(level) and call this function on button click passing the level as parameter.
- make the anonymous function a normal function and then call that function and pass the level to it so that others can use that level inside that function
index.html
Outdated
<div id="bttn"><button class="modal__close-button" onclick="closeModal()">Easy <i class="fa fa-xmark"></i></button><br></div> | ||
<div id="bttn"><button class="modal__close-button" onclick="closeModal()">Medium<i class="fa fa-xmark"></i></button><br></div> | ||
<div id="bttn"><button class="modal__close-button" onclick="closeModal()">Hard <i class="fa fa-xmark"></i></button><br></div> | ||
<div id="align_button"><button class="modal__close-button" onclick="closeModal()">Easy <i class="fa fa-xmark"></i></button><br></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, bad naming of classes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made all the changes you suggested.pl review.
index.html
Outdated
<div id="bttn"><button class="modal__close-button" onclick="closeModal()">Medium<i class="fa fa-xmark"></i></button><br></div> | ||
<div id="bttn"><button class="modal__close-button" onclick="closeModal()">Hard <i class="fa fa-xmark"></i></button><br></div> | ||
<div id="align_button"><button class="modal__close-button" onclick="closeModal()">Easy <i class="fa fa-xmark"></i></button><br></div> | ||
<div id="align_button"><button class="modal__close-button" onclick="closeModal()">Medium<i class="fa fa-xmark"></i></button><br></div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also the size of the button denoting the easy medium hard is not equal in size
index.js
Outdated
console.log() | ||
timer.innerHTML = "<b>" + counter + "</b>"; | ||
}, 1000); | ||
setInterval(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this here unused
padding: 0; | ||
box-sizing: border-box; | ||
} | ||
|
||
:root { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
color variables are set on a project level(by the maintainer). Remove these, you don't need it for this issue
… removed to avoid glitch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know it might be frustrating to get a review on the name of variables or class, but it's important.
you are doing good. stay motivated 👍
index.html
Outdated
@@ -23,16 +23,16 @@ <h4 class="time--stat" style="text-align:center"> | |||
</span> | |||
</h3> | |||
<div class="shadow" id="shadow"></div> | |||
<center><button class="primary-button" onclick="openModal()" >Start Game</button></center> | |||
<center><button class="primary-button" id="hide" onclick="openModal()" >Start Game</button></center> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hide is not a better name, try something like start-btn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the name from string to level
index.js
Outdated
modal.classList.remove("active"); | ||
shadow.classList.remove("active"); | ||
let counter = 0; | ||
function handleLevel(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use better parameter name, like level instead of string 🤣
index.js
Outdated
timerStart(); | ||
|
||
} | ||
function timerStart(){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I suggested in my previous comment, do not make this a separate function, there is this anonymous function just change it to a named function, and then instead of calling the timerStart(), call that function passing the level to it. The timer logic was already there in that function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not working when I'm doing this idk why :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Made changes !
one silly bug is, I can play the game without pressing the start game btn.
I modified you code(at commit do the following:
|
Done! thank you so much for helping! |
your code is working now, but it took quite some time, and this issue was assigned to someone else after you, so won't be able to merge this PR but will give you a |
Any updates? |
already gave you +5 bonus
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
work's fine
Added a start button and as the user clicks it, modal pop-ups which allows the player to choose the level of the game. Made changes according to previous UI only,