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

Natalie's HW 7 submission #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Natalie's HW 7 submission #3

wants to merge 1 commit into from

Conversation

natalieogle
Copy link

HW 7.

@@ -0,0 +1,59 @@
var MemoryGUI = (function () {
Copy link

Choose a reason for hiding this comment

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

Your game looks great! I like the cards and the added hover effect in your css. The code is laid out appropriately, and your GUI is just responding to the wishes of the games logic and not making its own decisions, which is perfect.

A few suggestions for enhancement:

Currently you can have a number of cards face up at a time, I think I managed to face up about 5-6 of them in one go before they started disappearing. Try tinkering with the timeout delay value, or possibly disabling clicking while two cards are face up.

Your code is pretty easy to read, especially considering that we're all aware of what we're about to walk into given that it's the homework! Try getting into the habit of leaving more comments however, as in the future this will help both you and anyone that needs to go over your code in knowing what they're looking at.

And finally, a quirky little bug!

Try clicking on one of your cards and then pressing reset. If you look above the cat's head you'll notice that the name of the previous card now appears above, faintly. This is regardless of whatever card happens to actually exist there. My assumption is that the bug lies in your reset function, which doesn't clear the inner html of the cards. Try to see if you can find out what's causing this!

Great work overall.

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.

3 participants