-
Notifications
You must be signed in to change notification settings - Fork 86
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.
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
Assignmnet 1 #7
base: main
Are you sure you want to change the base?
Assignmnet 1 #7
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work overall! You did an excellent job. Here are some callouts of things that I feel were particularly well done:
- Clean implementation with clear function naming, good use of
id
andclass
attributes with minimal inline styling - Clever implementation of the shuffle function, swapping the two songs in place is a good way to simulate song shuffling
- Well organized, clearly defined function naming
Some general thoughts on things for next time beyond the comments left:
- In the future for easy review, let's try to squash related commits to make for better version history using interactive rebase:
git rebase -i HEAD~n
. - Let's also try to add more descriptors for each commit beyond updating the file name itself. Some examples can be "Added function to shuffle through songs" or "Implemented playlist like button"
- Do a quick pass over the code to remove any unnecessary comments
<div class="playlist-cardS"></div> | ||
|
||
<!-- Modal Structure --> | ||
<div id="playlistModal" class="modal"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we typically use kebab-case for HTML attributes likeid
and class
let cardContainer = document.getElementsByClassName('playlist-cardS')[0]; | ||
let newCard = document.createElement('div'); | ||
newCard.classList.add("playlist-card"); | ||
newCard.innerHTML = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using a <template>
tag in html to define newCard
's HTML. this will keep your code simpler and easier to follow, with all the HTML in the respective file
<template id="card-template">
<img id='card-img' src=${playlist.playlist_art} alt=${playlist.playlist_name}>
<p id='expand-icon' class='expand-icon' data-playlist-id='${playlist.playlistID}'>ᐩ</p>
<h3 id='playlist-title'>${playlist.playlist_name}</h3>
<p id='creator-name'>${playlist.playlist_creator}</p>
<span class='likes'>
<p class='heart' id='heart' data-playlist-id='${playlist.playlistID}'>♡</p>
<span class='like-count' id='like-count' data-playlist-id='${playlist.playlistID}'>${playlist.likeCount}</span>
</span>
</template>
// in javascript
const template = document.getElementById('card-template').content;
const songElement = document.createElement('div'); | ||
songElement.classList.add('song'); | ||
|
||
songElement.innerHTML = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to above, a template might make this code a bit easier to follow
let playlistId = Number(event.target.getAttribute('data-playlist-id')); | ||
const playlist = data.playlists.find(p => p.playlistID === playlistId); | ||
if (playlist) { | ||
const songsCopy = [...playlist.songs]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
limit use of unnecessary variable assignments when not needed, for example, these two lines can be replaced with
const shuffledSongs = shuffleArray([...playlist.songs]);
@@ -2,5 +2,412 @@ body { | |||
font-family: 'Arial', sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider using comments to break the css file into sections for easier navigation, or splitting into multiple files as necessary
} | ||
|
||
|
||
.playlist-cardS { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: naming here is confusing and difficult to distinguish between .playlist-cardS
and .playlist-card
No description provided.