Skip to content

Project 1 Feedback (DO NOT MERGE ME) #1

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.

Sign up for GitHub

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

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

Project 1 Feedback (DO NOT MERGE ME) #1

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions feedback.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Project 1 Evaluation
[inline code comments]()
## Planning / Process / Submission
**1: Progressing**
>App is submitted, with basic evidence of planning. Documentation exists, but lacks common areas such as setup instructions, description of application functionality and link to deployed application
>Make sure to add a `readme.md` with appropriate documentation. Refer to the "Writing Documentation" lecture for guidance.

## Technical Requirements
**2: Performing**
>App functions with minimal or no errors and is deployed on github pages

## Code Quality
**3: Excelling**
>No major code quality issues, makes use of JavaScript best practices appropriately, and follows techniques such as separation of concerns, abstraction, and encapsulation

## Creativity and Interface
**3: Excelling**
>The app is fully responsive, incorporates outside technologies such as Flexbox. App incorporates modern UI themes, and adds unique flair.
3 changes: 3 additions & 0 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
<title>Simon Piano</title>
<link rel="stylesheet" href="style.css">
<link href="https://fonts.googleapis.com/css?family=Baloo+Thambi" rel="stylesheet">
<!-- Nice use of external web fonts -->
</head>
<body>
<div class="jumbotron">
Expand All @@ -13,6 +14,7 @@ <h1><span id="blue">S</span><span id="red">i</span><span id="yellow">m</span><sp
<ul class="piano">
<li>
<div id="keyLowC" class="white keyNotPress"></div>
<!-- for css classes, use spine-case (i.e. "key-not-press") -->
<div id="keyCS" class="black"></div>
</li>
<li>
Expand Down Expand Up @@ -48,6 +50,7 @@ <h1><span id="blue">S</span><span id="red">i</span><span id="yellow">m</span><sp
<button type="button" name="button" class="gameStart" id="buttonFreePlay">Free Play</button>
<button type="button" name="button" id="buttonReferenceC">Reference Low C</button>
<p id='scoreKeeper' class="score" >Score: <span id="score" class="score"></span></p>
<!-- For HTML, best practice is to always use double quotes (i.e. "score-keeper") -->
<p id="countDown">Count Down: <span id="timer"></span></p>
</div>
</div>
Expand Down
15 changes: 15 additions & 0 deletions main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ let timer = 0
let secondCount = document.getElementById('timer')

var buttonNormal = document.getElementById('buttonNormal')
// Vanilla JS? Nice...
buttonNormal.addEventListener('click', function () {
compArray = []
userArray = []
Expand All @@ -32,6 +33,8 @@ buttonExtreme.addEventListener('click', function () {
extreme = true
timeSubtracter = 30
compBox.randomize()
// Notice how this is repetitive of the normal mode? Think about abstracting it into
// a separate function with varying values passed in
})

var buttonFreePlay = document.getElementById('buttonFreePlay')
Expand All @@ -43,6 +46,7 @@ buttonFreePlay.addEventListener('click', function () {
// secondCount.innerHTML = timer
// }, 1000)
// }
// Remove commented-out / unused code from production branches
scoreCount = false
listen = true
userArray = []
Expand Down Expand Up @@ -98,6 +102,9 @@ class Key {
userArray.push(this.num)
this.compare()
}
// Does the below functionality need to be attached to each `Key` instance since it
// really pertains to the game as a whole? Think about moving this functionality to the
// `compBox` or another helper class.
compare () {
if (scoreCount) {
let userString = userArray.join('')
Expand All @@ -109,6 +116,8 @@ class Key {
userArray = []
compArray = []
time = 800
// Think about abstracting this code that runs when they get it wrong into
// its own function / method
}
}
if (userArray.length === compArray.length) {
Expand All @@ -122,6 +131,7 @@ class Key {
setTimeout(() => {
compBox.randomize()
}, time * 1.5)
// Same here
}
}
}
Expand Down Expand Up @@ -159,3 +169,8 @@ let keys = [
new Key('keyB', 'bnote', 6),
new Key('keyHighC', 'highCNote', 7)
]

// Awesome job encapsulating functionality into classes. I think a little more abstraction
// (more narrowly defined functions / methods in terms of functionality) and potentially
// creating a second helper class (class Game ?) would be the only improvements that could
// be made. Overally, really great job.
7 changes: 7 additions & 0 deletions style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ li {

.black {
z-index: 10;
/*Is 10 neccessary, wouldn't 1 work?*/
display: inline-block;
width: 60px;
height: 240px;
Expand Down Expand Up @@ -118,3 +119,9 @@ h1 {
#red {
color: #EE2C2C;
}

/*Really great formatting and semantic naming for your classes. To strictly follow
best practice, order your selectors by specificity:
- All general element selectors first
- Then all class selectors
- Then all id selectors */