kalebmcc / Simon_Game

A web application that allows you to play a color sequence memory game.
https://kalebmcc.github.io/Simon_Game/
0 stars 0 forks source link

Feedback #1

Open esin87 opened 3 years ago

esin87 commented 3 years ago

Project 1 Feedback

Code Quality - Performing

Criteria: Is the code well formatted? Are variable and function names semantic and sensible? Is the code easy to read and understand?

Strengths:

Areas for Growth:

function addColor() {
    //use const instead of let for any variable not intended to be reassigned
    const randomIndex = Math.ceil(Math.random() * 4);
    // instead of a long if/else sequence, just pass the number to colorSequence
    // then you can use the indices to grab the right element out of the colors array to light up
    selectButton(colors[randomIndex - 1]);
    colorSequence.push(randomIndex);

    // if (randomIndex === 1) {
    //  selectButton(blue);
    //  colorSequence.push(1);
    // } else if (randomIndex === 2) {
    //  selectButton(red);
    //  colorSequence.push(2);
    // } else if (randomIndex === 3) {
    //  selectButton(yellow);
    //  colorSequence.push(3);
    // } else if (randomIndex === 4) {
    //  selectButton(green);
    //  colorSequence.push(4);
    // }
}

In a similar vein, how could you DRY up your loop function?

.difficulty-menu__btn {
    height: 10%;
    border: 0;
    color: white;
    border-radius: 4px;
}

.difficulty-menu__btn .difficulty-menu__btn--easy {
    background: lightskyblue;
}

.difficulty-menu__btn .difficulty-menu__btn--med {
    background: gold;
}

.difficulty-menu__btn .difficulty-menu__btn--hard {
    background: lightcoral;
}

The corresponding markup would look like this:

<div class="difficulty-menu">
    <button class="difficulty-menu__btn difficulty-menu__btn--easy" id="easy">
        EASY
    </button>
    <button class="difficulty-menu__btn difficulty-menu__btn--med" id="medium">
        MEDIUM
    </button>
    <button class="difficulty-menu__btn difficulty__btn--hard" id="hard">
        HARD
    </button>
</div>

Note that we're using classes to select elements to style, and reserving the ids for JS DOM manipulation. You can name your classes in any way that makes sense and is sustainable -- the pattern I used here is something called Block Element Modifier (BEM), but there are many more.

Technical Requirements - Excelling

Criteria: How does the project stack up to the requirements for this project? Is the developer making use of the material we've covered in a way that makes sense?

Strengths:

Areas for Growth:

Creativity and Interface - Performing

Criteria: Is the application easy to navigate? Does it work well in every major browser? Is it responsive? Does it incorporate modern UI Themes?

Strengths:

Areas for Growth

Functionality - Excelling

Criteria: Does the application work without errors or bugs? Does it present a complete app, where every feature is fully implemented in a way that makes sense?

Strengths:

Areas for Growth:

Presentation - Performing

Criteria: Is there adequate documentation? Is the repository well organized and free of clutter?

Strengths:

Areas for Growth:

Hard Requirements - Complete:

Necessary Deliverables - Complete:

Grade: Pass :tada: :tada: :tada:

Kaleb, excellent work on this project! Super impressed at all the effort you put into creating this deluxe, modern version of Simon. The design is clean and professional, the code is easy to read, and the functionality is above and beyond MVP. There are some areas to revisit as you continue working on this project, such as refactoring your HTML to be a bit more semantic, making your CSS rules more efficient, and DRYing up your JavaScript, but overall, you did a fantastic job. Thank you for all your hard work on this, and for delivering a project that goes above and beyond the requirements. Can't wait to see your continued progress in the course!

esin87 commented 3 years ago

One more thing I forgot to add -- it's faster to add Google fonts via link elements in HTML than in a CSS @import. Just be sure to add the links before your own stylesheet so that font names are accessible, and be sure to grab the pre-connect link as well.