Skip to content
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

Music Album Library by Joyce Kuo, Johanna Eriksson, Liselotte Engström #44

Open
wants to merge 45 commits into
base: main
Choose a base branch
from

Conversation

JoyceKuode
Copy link

Netlify link

https://jojolialbumlibrary.netlify.app/

Collaborators

Johanna Eriksson, Liselotte Engström
[joheri1, IamLise]

Copy link

@Fannyhenriques Fannyhenriques left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Way to go, Liselotte, Joyce and Johanna! You have created a REALLY cool website!

Loved the styling and color scheme. I especially loved the way you designed the filter and sort menus to come in from the right!! 🎉

Great job dividing the filtering function so that users can choose based on genre and artist!

Overall, the website seems to be functioning as it should, and the code is very well-structured. For example, all the event listeners are grouped together in one place, while the functions are organized separately, making it easy to understand and follow. It’s also really clever how you added functions for responsiveness! This approach makes the code much cleaner.

I believe Jennie mentioned that it’s good practice to have arrays at the top of the JS.file, but other than that, the JavaScript looks great!

Amazing job, everyone! We are super impressed!

/Fanny, Zoe, and Kelly.

Comment on lines +302 to +309
// Function to check screen size and add/remove mobile class
const checkScreenSize = () => {
if (window.innerWidth <= 768) {
document.body.classList.add('mobile')
} else {
document.body.classList.remove('mobile')
}
updateEventListeners()

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super nice!!

Comment on lines +29 to +34
// Function to close panels with a delay
const closePanelWithDelay = (panel, button, delay) => {
setTimeout(() => {
closePanel(panel, button)
}, delay)
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was very clever!

Copy link

@JennieDalgren JennieDalgren left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job with this project!
Nice to see that you have really focused on understanding array methods like sort and filter. That was the main learning this week

You have met all requeiremnts and I don't really have that much that you need to improve.
Remember to clean up the code and only leave the comments you need.
It seems you have a comment //do we need this ? next to genreSelect. If you aren't using genreSelect, consider removing it to keep the code clean.

You have a lot of code - next step for you would be to see how to refactor and make the code even more reusable. can you group together some of the css maybe?

nice work with naming of variables, functions and nice animations for that panel that slides in and out.

For accessibility - add an alt text to your objects, to use as the alt for your image tags.

Also, keep the array with data either in its own file (preferably) or at the top of the code file. You can also delete the folders with images that you are not using in your project.

Keep up the good work! ⭐

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants