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

guess-who-elbines #346

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

guess-who-elbines #346

wants to merge 20 commits into from

Conversation

elbines
Copy link

@elbines elbines commented Sep 11, 2023

No description provided.

Copy link

@olgalepisto olgalepisto left a comment

Choose a reason for hiding this comment

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

I love that you have a Taylor Swift theme for your game now! I just listened to folklore today :D
The game seems to work as it should, and I like that you comment on your code very comprehensively. I made some minor suggestions, but great job overall :)

//it "Grabs" the board element and changing the inner html
board.innerHTML +=
//use the information in the array (charactersInPlay) to shoe information about people.
//${person.name} the value of all the names in array (CHARACTERS)

Choose a reason for hiding this comment

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

The code is really well commented throughout so it's easy to keep up as a reader!


currentQuestion = {
category: category,
// value: value
value: value,
textForAlert: textForAlert, //S

Choose a reason for hiding this comment

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

Just a suggestion, but I'm wondering if textForAlert could've been named in a more specific name to fit the context it's used for, maybe songType or chosenValue?

code/index.html Outdated

<img class="guess-who-icon" src="images/guess-who-bubble.png" alt="Guess Who" />

<h1>Does the person have</h1>

Choose a reason for hiding this comment

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

This line could be changed to a title that refers to your theme :)

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.

2 participants