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 a modal allowing the player to choose the level and a start gam… #21

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

nematanya
Copy link

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,

@dev-lovedeep
Copy link
Contributor

mention the issue number

Copy link
Contributor

@dev-lovedeep dev-lovedeep left a 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 {
Copy link
Contributor

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 Show resolved Hide resolved
style.css Outdated
font-size: 2rem;
}

.p:not(:only-child) {
Copy link
Contributor

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{
Copy link
Contributor

Choose a reason for hiding this comment

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

use better names

Copy link
Author

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
Comment on lines 94 to 99
*::before,
*::after {
margin: 0;
padding: 0;
box-sizing: border-box;
}
Copy link
Contributor

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 *{ }

Copy link
Author

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 !

@nematanya
Copy link
Author

mention the issue number

Issue no.#7

@nematanya
Copy link
Author

center the position of modal, add game rules in it. Also note that the timer should not start before the game actually start

Trying to change it !

@nematanya
Copy link
Author

center the position of modal, add game rules in it. Also note that the timer should not start before the game actually start

fixed this issue

Copy link
Contributor

@dev-lovedeep dev-lovedeep left a 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:

  1. if I do not select the level and close the modal by clicking outside it , it is also starting the game.
  2. 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:

  1. use the index.js from previous commit, because you have changed quite a lot of thing
  2. create a function handleLevel(level) and call this function on button click passing the level as parameter.
  3. 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>
Copy link
Contributor

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

Copy link
Author

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>
Copy link
Contributor

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();
Copy link
Contributor

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 {
Copy link
Contributor

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

Copy link
Contributor

@dev-lovedeep dev-lovedeep left a 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>
Copy link
Contributor

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

Copy link
Author

Choose a reason for hiding this comment

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

OK

Copy link
Author

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)
Copy link
Contributor

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(){
Copy link
Contributor

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

Copy link
Author

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 :(

Copy link
Author

Choose a reason for hiding this comment

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

Made changes !

@dev-lovedeep
Copy link
Contributor

dev-lovedeep commented Oct 5, 2022

one silly bug is, I can play the game without pressing the start game btn.

It's not working when I'm doing this IDK why :(

I modified you code(at commit ab28af1) a little, and it was working correctly (you are very close)

do the following:

  1. change anonymous function (()=>{....}) to something like const game = (level)=>{...}
  2. put the timer code back where it was inside the game function
  3. only keep handleLevel(level) ,closeModal(),hideButton(),openModal() outside

@nematanya
Copy link
Author

  1. const game = (level)=>{...}

Done! thank you so much for helping!

@dev-lovedeep
Copy link
Contributor

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 +5 bonus point for your work and will give accepted-tag later

@nematanya
Copy link
Author

nematanya commented Oct 7, 2022

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 +5 bonus point for your work and will give accepted-tag later

Any updates?

@dev-lovedeep
Copy link
Contributor

already gave you +5 bonus

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 +5 bonus point for your work and will give accepted-tag later

Any updates?

@dev-lovedeep dev-lovedeep reopened this Oct 31, 2022
Copy link
Contributor

@dev-lovedeep dev-lovedeep left a comment

Choose a reason for hiding this comment

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

work's fine

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants