(What did you set out to accomplish this module?)
- A: End-of-Module Assessment:
- B: Individual Work & Projects:
- C: Group Work & Projects:
- D: Professional Skills:
- E: Community Participation:
PASS
Palette Picker is a single page application that allows a user to save randomly generated color palettes to a backend built with Node.js and Express. The front end application was built using jQuery. Users are able to create new projects and palettes, as well as delete palettes from the database.
58 points: (50 possible points) No approach was taken that is counter to the spirit of the project and its learning goals. There are no features missing from above that make the application feel incomplete or hard to use. Data persists on refresh using a knex/postgreSQL database.
- Nice implementation of the extension - but the UI for it is a little funky. Would like some visual indicator that you can edit that hex code value. (Maybe a little 'edit'/'pencil' icon?). Additionally, if I make the HEX value white, I lose the ability to see the HEX value text. Would be good to change that text to a dark color when white is the chosen color.
17 points: (20 possible points) User interface is mostly intuitive, though the instructor might need some guidance on interactions. Styling is mostly consistent, but could use some clean up.
-
I like the vertical layout of the colors for each palette but on my screen it cuts off after the 4th one and I have to scroll down to see the 5th color. Would be nice to set a
height: 100%on the containing element here to ensure it's always the height of a user's screen no matter what size their browser is. -
I'm seeing a weird bug where the 5th color stays in place (even when it's not locked). When I click on a pre-existing palette, the 5th color doesn't change.
-
I like the handwriting font, but I wouldn't use it everywhere. With smaller elements, stick to something a little more rigid and basic. Fancier fonts are great for bigger headings rather than tiny button text.
17 points: (20 possible points) Project has a running test suite that tests every server-side endpoint with many happy and sad path cases.
-
Don't particularly need all of these assertions, they are all doing the same thing -- asking if there a resource at a bad URL
-
Curious how this test is working. If it should be returning a single palette, the response shouldn't be an array, it should be an object. Additionally, if the length of the array is 1, there wouldn't be an element at
response.body[2]. There would only be an element at index 0. -
Would be good to assert against an error message here as well, not just a status code
-
You wouldn't send through an id during a POST request naturally, so don't do it in your tests either. Just send through an empty object.
10 points: (10 possible points) Each line of the server file (on a separate branch) is commented and explains the code using precise, correct terminology and specificity
22 points: (30 possible points) Application is thoughtfully put together with some duplication and no major bugs. Developer can speak to choices made in the code and knows what every line of code is doing.
-
This isn't really doing anything meaningful for you, and can sometimes be confusing when you're really returning the index file on your root get request through express.static. I'd get rid of this.
-
Is this check necessary? Would it not return an empty array by default if there were no records?
-
Would be nice to break this type of error handling out into a helper file. You're repeating it through a lot of your server file, DRY this up a bit.
-
Send back the id in this error message as well just to give the user an indicator of what they actually tried to delete. I'm also torn about this being a 422 rather than a 404. A 404 literally means a resource doesn't exist at that endpoint, a 422 is generally used more for user error when you're passing through a formatted body object.
-
Fetch requests aren't dependent on the DOM. So they don't need to wait for document ready. Kick off this request and get that data ASAP.
-
Why is there this lonely .catch here?
-
Was it supposed to go here instead? ;)
-
You don't want to be appending to the DOM on each iteration of a for loop. DOM manipulations are expensive. Instead, you'd want to use Document Fragments to build up all the HTML you want to append, and then add it to the DOM all at once at the end of the loop.
-
This is a poor naming convention for a method. Function names should be actions/verbs, like
appendProject. Variables of any other data type should be nouns. -
This is a lot of arguments to be passing into a function. When you argument lists get long like this, it can be really easy to mess up their order and pass things in at the wrong spots. I'd refactor this to just take in a single object.
-
You could have just added a
unique()flag in your database schema that would do this check for you automatically.
17 points: (20 possible points) Developer(s) make many small, atomic commits that clearly document the evolution of the application, sometimes contain irrelevant changesets. Commit messages are concise and consistent in syntax and tense. There are no instances where the developer(s) have committed source code that should be .gitignored.
-
Nice, consistently formatted commit messages, even when we're working fast during code alongs.
-
Try working with github issues to keep track of your to-do list for functionality/bug fixes/design issues/etc. Pull requests aren't necessary when you're working individually, they're more for code review when you have other teammates that need to approve your changes.
-
Seems like a lot of extra/irrelevant changes are making it into your commits, like here. Try to keep the diffs a bit tinier. I'd say for a project of this size and scope, you probably want at least 100 commits.
Damnnnnnnnn, girl, good job.
(ways you supported the larger Turing community)
( Leave blanks for reviewers )
( Leave blanks for reviewers )