-
Notifications
You must be signed in to change notification settings - Fork 357
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?, Nina Berggren #207
base: main
Are you sure you want to change the base?
Conversation
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 has been a fun and interesting task to review your Guess-Who project. I think you have made a great job with the project. No doubt you game works and fulfills the requirements. As far as I know you are following the guidelines and I think your code is well-described.
In addition to my comments a few things I would like to mention are:
- Good job with you own logo, choice of colours and css styling.
- A plus for the adjusted sizes of the cards and other adjustments, to better suit the mobile-first approach (as mentioned in your demo).
This week we are building a classic "Guess Who" game and digging deaper into JavaScript | ||
with objects, arrays and functions. This weeks project was really difficult. | ||
|
||
Some of the learnings: | ||
|
||
- How to plan and think about game logic | ||
- Create and manipulate loops, objects and arrays. | ||
- Array methods such as `forEach()` and `filter()` | ||
- Use built in functions, map and math.random etc. | ||
- the differents between "including value" and "value" | ||
- More about manipulating the DOM using Javascript | ||
- How to structure your code in functions |
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.
This is a clear and structured way of writing a Read Me, very good example I would say. :)
<option value="brown">brown hair</option> | ||
<option value="yellow">yellow hair</option> | ||
<!-- Add the rest of hair colors as options --> | ||
<option value="black">⚫️ black hair</option> |
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.
Good idea adding the color dots and symbols/emojis to the options in the drop down selection, to make options easier to find / choose between.
<option value="hat">🎩👒🧢 hat</option> | ||
<option value="a hood">has a hood</option> | ||
<option value="earrings">💎 earrings</option> | ||
<option value="necklace">⛓ necklace</option> |
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.
Lots of good examples added to the option groups acessories and other!
alert(`Good guess! Yes, the person has ${value} ${category}! We'll keep all who have ${value} ${category}`) | ||
charactersInPlay = charactersInPlay.filter((person) => person[category] === value) | ||
} else { | ||
// alert popup that says something like: "No, the person doesnt have yellow hair! Remove all people with yellow hair" | ||
alert(`Oh no! the person doesn't have ${value} ${category}! We'll remove all who have ${value} ${category}`) | ||
charactersInPlay = charactersInPlay.filter((person) => person[category] !== value) |
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.
This is a smart way to code the alert for both the "eyes" and "hair"-category together, to avoid writing them out in separate sections as in the examples above for "others" and "accessories".
Would it maybe be possible to do the same thing for the "other" and "accessories" - sections?
@@ -1,28 +1,47 @@ | |||
/* Global css variables used for colors */ | |||
|
|||
/* Offwite #d8faf4 |
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.
Good idea to write all the color-codes here for easy access.
/* The div with the guess class is only shown on hover */ | ||
.card:hover .guess { | ||
display: flex; | ||
} | ||
|
||
.guess span { | ||
font-size: 16px; | ||
font-size: 1.2rem; |
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 "rem" for the font-size?
|
||
## The problem | ||
|
||
Describe how you approached to problem, and what tools and techniques you used to solve it. How did you plan? What technologies did you use? If you had more time, what would be next? | ||
I know of the game, we have it at home and play alot, that was the easy part. I started whit the pre written code, trying to find out what was missing, what the different steps should be and goggled (W3 and MDN), with "trial and error" found a way forward to a solution. I have struggled with the filter functions and arrays. |
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 a clear explanation of your process for the project, your challenges and what the next steps would be if you had more time to build on this project.
|
||
<select title="questions" id="questions"> | ||
<option value="" selected disabled>Select attribute</option> |
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.
Good choice to add this "non-option" as the first message shown before any of the options are selected.
No description provided.