search

Gabriels-Support-Team / Organization-Music-Explorer

Starter code for the Music Playlist Explorer project
0 stars 0 forks source link

Project 1 Feedback - Kevin Zhu #2

Open kevinzhu00 opened 5 months ago

kevinzhu00 commented 5 months ago

Feedback

Hey Gabriel, good job on your first project. The code meets majority of the listed requirements, and in some aspect it seems you went above the necessary requirements and even added some features that would be nice to have. I would first like to echo Samy's comments regarding sticking to the project specs. Here is some of my feedback (I tried not to be redundant)

Code

  1. While this is a simple project that you can do in a few commits, it is important to document what each commit is doing. In the future, other people will look at your code commits, by providing descriptive messages it will make debugging easier to identify the errors in the code. (i.e in your commit "almost final changes" provides little detail about the contents of the commit, Instead add a comment about updating Featured button etc.)
  2. Keep consistency with the formatting of your code. Whether this be Javascript or Html it is important to keep consistent line spacing and indent. If you would like help with this, your text editor should have a way to auto-format your code.
  3. Do not hardcode data values in your code. When generating the features page, you hardcoded the number of playlists to choose from. But what if that data were to change (i.e. remove a playlist)?
  4. (Nitpick) Provide detailed and distinct class names for your CSS classes. While close-btn will not cause any issues, there is a similar btn-close class found in Bootstrap. Also, the name close-btn does not relate to all its uses.

Features

  1. I appreciate how you implemented a back button in the featured page, or a way to navigate out of the back button. However, as the Project specs mentioned the feature page should be like a "home page". Instead of a back button, a button to navigate to My Playlists could be more appropriate.
  2. You attempted to implement the edit button and like you mentioned the functionality is incomplete. In general, if you have not completed the functionality of a feature it should not be pushed to master. I'd recommend saving the code in another branch for you to come back to, especially when your future projects have multiple features to be implemented at once.
  3. I like the color scheme that you added to personalize the website. However, the green X to remove a song from the playlist may be a little confusing.
  4. (Nitpick) As demonstrated in the screenshots, the song name in the playlist is bolded, compared to the song artist. By differentiating the two it will improve usability.
Capn05 commented 5 months ago

Key takeaways for next weeks project: make sure commit messages are descriptive and useful to others reading them Keep code formatted nicely Don't use hardcoded data, this is not flexible to changes Keep project consistent with requirements in terms of User Interface In general don't push code that is incomplete (I will say that for this project we do get partial credit for at least having an edit button that is why i Included it)