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 Release Project #105

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

Music Release Project #105

wants to merge 22 commits into from

Conversation

gittebe
Copy link

@gittebe gittebe commented Oct 13, 2024

@JennieDalgren JennieDalgren self-assigned this Oct 17, 2024
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.

Good job, you meet all requirements!

Good structure of your project, it includes several components, such as Album.jsx, CoverImage.jsx, AlbumName.jsx, and ArtistName.jsx.

Interesting that you've tried out prop-types. How come? And how did you feel about it? We will learn more about typing when we learn about TypeScript. Feel free to use prop types if you want to.

Really nice implementation of the sidebar aswell and the different scrolling of the sections.

Overall Feedback:
You've done an excellent job at structuring your project into components and passing props efficiently. Each component is focused on a specific task, and the use of PropTypes is a strong practice for type safety.

This is a very well-organized and solid project! Keep up the great work, you're really mastering React fundamentals!

Stay golden ⭐

Comment on lines 17 to 19
<CoverImage
coverImg={album.images.length > 0 ? album.images[0].url : 'fallback-image-url.jpg'}
albumUrl={album.external_urls.spotify}

Choose a reason for hiding this comment

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

great to use a fallback image

import PropTypes from 'prop-types'

// Album Name
export const AlbumName = ({ name, albumUrl }) => {

Choose a reason for hiding this comment

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

Good practise to only pass along the data that this component needs to know about.

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.

2 participants