sarah-yu · perryf · Nov 4, 2017
diff --git a/README.md b/README.md
@@ -3,7 +3,7 @@
 Simon Learns Chinese is based on the concept of the memory game, Simon, and inspired by gamified language learning apps such as Duolingo. The purpose of the game is to test the user's memory while allowing the user to gain a basic but intuitive understanding of the Chinese language through repeated exposure to simple vocabulary and grammar patterns.
 
 Play it here: https://sarah-yu.github.io/simon
-
+<!-- Consider formatting your link like this: [Simon Learns Chinese](https://sarah-yu.github.io/simon) -->
 
 ## Current Features
 
@@ -47,3 +47,6 @@ Play it here: https://sarah-yu.github.io/simon
 ## Contribute
 
 Source code: https://github.com/sarah-yu/simon
+<!-- Here you could prompt users to submit a pull request with suggestions, and have a link to your issues page for questions and comments -->
+
+<!-- Good job on the README!  You hit all the marks and were thorough without being too verbose. -->
diff --git a/feedback.md b/feedback.md
@@ -0,0 +1,18 @@
+# Project 1 Evaluation
+[inline code comments]()
+## Planning / Process / Submission
+**2.5: Performing**
+>Submission contains clear evidence of planning, adequate documentation, include all from previous category, as well as additional information such as unsolved issues.
+
+## Technical Requirements
+**2.5: 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
+**2.5: Excelling**
+>The app is fully responsive, incorporates outside technologies such as Flexbox. App incorporates modern UI themes, and adds unique flair.
+> Amazing creative spin on Simon.  Only reason I did not give this a 3 was because of sizing issues.  Look into media queries!
diff --git a/index.html b/index.html
@@ -3,15 +3,18 @@
   <head>
     <meta charset="utf-8">
     <link href="https://fonts.googleapis.com/css?family=Karla:400,700" rel="stylesheet">
+    <!-- Nice job using google fonts -->
     <link rel="stylesheet" href="style.css">
     <title>Simon Learns Chinese</title>
   </head>
   <body>
     <header>
       <ul>
         <li><button class="start-page">Start Page</button></li>
+        <!-- Consider renaming this `Start Menu` ^ -->
         <li class="game-info" id="current-level"></li>
         <li class="site-logo">Simon Learns Chinese</li>
+        <!-- Consider making this a heading ^ -->
         <li class="game-info">Score: <span id="score">0</span></li>
         <li><button id="reset-score">Reset Score</button></li>
       </ul>
@@ -33,3 +36,5 @@
     <script src="js/script.js"></script>
   </body>
 </html>
+
+<!-- HTML is solid with good organization.  Nice job using correct casing (`kabob-case`) for attribute names.  Also good job using semantic sections like header, main, and section.  -->
diff --git a/js/script.js b/js/script.js
@@ -1,12 +1,15 @@
 $('document').ready(function() {
+	// You dont really need document.ready here since your script file is already at the bottom of your html
 	let gameBoard = $('#game-board')
 	let wordBank = $('#word-bank')
 	let startPageEl = $('.start-page')
 	let resetScoreButton = $('#reset-score')
 	let currentLevel = $('#current-level')
 	let scoreEl = $('#score')
 	let score = 0
+	let timeToGuess // Declare this at the top to avoid console error when you click `Start Page`
 	const words = []
+	// Good job defining your variables at the top of the page
 
 	words.push(yiGe)
 	words.push(liangGe)
@@ -58,6 +61,8 @@ $('document').ready(function() {
 	words.push(fanQie)
 	words.push(huangGua)
 	words.push(hanBaoBao)
+	// Couple thoughts here.  One, consider moving this ^ into it's own seperate js file, or just into word.js
+	// Also, in word.js, you could put all of the words in the different parts of speech into their own arrays (subjects, verbs, etc).  Then you could just run a forEach over each of those arrays, pushing each of the words into the `words` array.
 
 	const measureWords = words.filter(word => {
 		return word.partOfSpeech === 'measureWord'
@@ -78,6 +83,7 @@ $('document').ready(function() {
 	const adjectives = words.filter(word => {
 		return word.partOfSpeech === 'adjective'
 	})
+	// Good use of filter, though I think a lot of this could be avoided if you just put each of the parts of speech words in their own arrays to begin with.
 
 	let level1 = {
 		level: 1,
@@ -106,11 +112,12 @@ $('document').ready(function() {
 	startPage()
 
 	function startPage() {
-		gameBoard.append(`
-		<div class="welcome-message">
-			<p>Test your memory skills while learning Chinese!</p>
-			<p>Select a level to start the game.</p>
-		</div>`)
+		gameBoard.append(
+			`<div class="welcome-message">
+				<p>Test your memory skills while learning Chinese!</p>
+				<p>Select a level to start the game.</p>
+			</div>`
+		)
 
 		wordBank.append(
 			`<div class="level-buttons">
@@ -120,12 +127,10 @@ $('document').ready(function() {
 			</div>`
 		)
 
-		$('.level-buttons')
-			.children()
-			.on('click', e => {
-				let userLevel = e.target.id
-				game(userLevel)
-			})
+		$('.level-buttons').children().on('click', e => {
+			let userLevel = e.target.id
+			game(userLevel)
+		})
 	}
 
 	function game(userLevel) {
@@ -141,6 +146,7 @@ $('document').ready(function() {
 				break
 		}
 	}
+	// Good use of swtich statements
 
 	function startLevel(level) {
 		currentLevel.text(`Level: ${level.level}`)
@@ -159,18 +165,17 @@ $('document').ready(function() {
 	function createGameBoard(level) {
 		for (let i = 0; i < level.pos.length; i++) {
 			gameBoard.append(`<div class="the-sentence-pos"></div>`)
-			$('.the-sentence-pos')
-				.eq([i])
-				.append(
-					`<div class="the-sentence-pos-label">${level.posGBLabel[i]}</div>`
-				)
+			$('.the-sentence-pos').eq([i]).append(
+				`<div class="the-sentence-pos-label">${level.posGBLabel[i]}</div>`
+			)
 		}
 	}
 
 	function createTheSentence(level) {
 		for (let i = 0; i < level.pos.length; i++) {
 			let ptOfSpeech = level.posProperty[i]
 			theSentence[ptOfSpeech] = getWord(level.pos[i])
+			// Consider declaring `theSentence` as a global variable at the top of the page.  Also I think renaming it something like `currentSentance` or something like that might make a little more sense.
 		}
 	}
 
@@ -182,13 +187,14 @@ $('document').ready(function() {
 	function displayTheSentence(level) {
 		for (let i = 0; i < level.pos.length; i++) {
 			let ptOfSpeech = level.posProperty[i]
-			$('.the-sentence-pos').eq([i]).prepend(`
-				<div class="word-tile">
+			$('.the-sentence-pos').eq([i]).prepend(
+				`<div class="word-tile">
 					<span class="word-info word-pinyin">${theSentence[ptOfSpeech]
 						.pinyin}</span>
 					<span class="word-info word-cn">${theSentence[ptOfSpeech].cn}</span>
 					<span class="word-info word-en">${theSentence[ptOfSpeech].en}</span>
-				</div>`)
+				</div>`
+			)
 		}
 	}
 
@@ -209,10 +215,11 @@ $('document').ready(function() {
 
 	function createWordBank(level) {
 		for (let i = 0; i < level.pos.length; i++) {
-			wordBank.append(`
-			<div class="wb-pos">
-				<div class="wb-pos-label">${level.posWBLabel[i]}</div>
-			</div>`)
+			wordBank.append(
+				`<div class="wb-pos">
+					<div class="wb-pos-label">${level.posWBLabel[i]}</div>
+				</div>`
+			)
 			$('.wb-pos-label').append('<div class="wb-tiles"></div>')
 		}
 
@@ -239,8 +246,7 @@ $('document').ready(function() {
 			window['wb' + ptOfSpeech][0] = theRightWord
 			window['shuffleWb' + ptOfSpeech] = level.pos[i].slice()
 			window['shuffleWb' + ptOfSpeech].splice(
-				window['shuffleWb' + ptOfSpeech].indexOf(theRightWord),
-				1
+				window['shuffleWb' + ptOfSpeech].indexOf(theRightWord), 1
 			)
 
 			getRandomWords(window['shuffleWb' + ptOfSpeech], 3)
@@ -255,6 +261,7 @@ $('document').ready(function() {
 		}
 		displayWBTiles(level)
 	}
+	// Hmm, you shouldn't need to read or write anything on the window object.  Keep your current wordbank in your JS file and update it for each question.
 
 	// reference: https://stackoverflow.com/questions/11935175/sampling-a-random-subset-from-an-array
 	function getRandomWords(array, size) {
@@ -324,6 +331,7 @@ $('document').ready(function() {
 			})
 		}
 	}
+	// Consider breaking userGuess up into 2 or 3 smaller more specific funtions
 
 	function checkSentence(level) {
 		if (theSentence.isEqual(userSentence)) {
@@ -366,3 +374,16 @@ $('document').ready(function() {
 		scoreEl.text(0)
 	})
 })
+
+
+// Great job on the project!  Your approach to functional programming is very solid and well organized.  Most functions are small and precise.  There are a few spots that I mentioned above that could improve the quality of your code, the biggest ones being...
+// - Pushing the individual words into an array could be done more systematically and logic could really be abstracted to another file.
+// - You shouldn't need to manipulate or read data through the window object.  Keep data local and update the page through HTML as needed.
+
+// Here are a few other suggestions/features to make moving forward...
+// - Something to happen when a user makes an incorect guess, or a `next question` button (If I have no idea what the sentence is on level 3, the only way to move forward is by guessing a ton of words)
+// - Maybe a submit button for each sentence
+// - Have the timer count down from 5 to 0
+// - Change `Start Page` button to `Start Menu` or something like that.
+
+// Overall this is a very strong project and you really went above and beyond.  I can see something like serving a real need in the real word.  Great job!
diff --git a/js/sentence.js b/js/sentence.js
@@ -22,6 +22,7 @@ class Sentence {
 	checkLevelThreeSentence() {
 		return this.checkLevelTwoSentence() && userSentence.adjective != null
 	}
+	// Nice way of keeping things concise! ^
 
 	isEqual(userSentence) {
 		let correct = false
@@ -53,3 +54,6 @@ class Sentence {
 
 let theSentence = new Sentence()
 let userSentence = new Sentence()
+// Declare these in the script.js file to make script.js easier to follow along with
+
+// Great use of classes here.  Methods are well defined.  You could arguably abstract some of the isEqual method into smaller more defined methods, but great job overall here!
diff --git a/js/word.js b/js/word.js
@@ -6,6 +6,7 @@ class Word {
 		this.partOfSpeech = partOfSpeech
 	}
 }
+// Nice use of a class here!
 
 // partOfSpeech: measureWord
 const yiGe = new Word('一个', 'one', 'yí gè', 'measureWord')
@@ -18,6 +19,7 @@ const qiGe = new Word('七个', 'seven', 'qī gè', 'measureWord')
 const baGe = new Word('八个', 'eight', 'bā gè', 'measureWord')
 const jiuGe = new Word('九个', 'nine', 'jiǔ gè', 'measureWord')
 const shiGe = new Word('十个', 'ten', 'shí gè', 'measureWord')
+// Talked more about this in script.js, but consider putting the parts of speech into their own arrays to make data easier to manipulate.
 
 // partOfSpeech: subject
 const wo = new Word('我', 'I', 'wǒ', 'subject')

diff --git a/style.css b/style.css
@@ -2,6 +2,7 @@ html,
 body {
 	color: #3a3a3b;
 	font-family: 'Karla', sans-serif;
+	/*Consider using an additional backup font between `Karla` and `san-serif`*/
 	height: 100%;
 }
 
@@ -64,6 +65,8 @@ header button:hover {
 
 .site-logo {
 	font-size: 2vw;
+	/*I think it makes sense to make the font-size here fixed, since the title is the only thing that is changing sizes when the page gets smaller*/
+	/*Also, consider calling this `title` instead or even better, just using a heading*/
 	font-weight: 700;
 	margin-left: auto;
 	margin-right: auto;
@@ -211,3 +214,9 @@ main {
 	color: #fff;
 	background: #cd6155;
 }
+
+/*Good job keeping things well organized and easy to read.  You've chosen good semantic class/id names to make things intuitive to understand.  Typically with CSS you'll want to go from least specific at the top to most specific at the bottom, so (elements, then classes, then ids).  However, since you've provided commented titles, this is also an acceptable way to do it.  For larger CSS files, and yours is starting to verge in that territory, consider breaking up the sections (header, main, wordbank, etc) into their own seperate CSS files.*/
+
+/*Sizing is good to a point.  Nice work making use of flex-box.  For more dynamic sizing, look into using *media queries* that can reformat and apply different CSS rules when your page gets smaller than a certain size.  https://www.w3schools.com/css/css_rwd_mediaqueries.asp  This could really help your page when it the screen size becomes small. */
+
+/*Aesthetically speaking, you've done a great job choosing the color palette, using rounded buttons, making use of `hover`, and chosing suitable font styling and size.  Consider adding a small transition time when user hovers over a button.  Page is pleasing to look at, easy to understand what's going on, and not too busy. */