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 Releases by Joyce Kuo #104

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

Conversation

JoyceKuode
Copy link

@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.

Great job with the project, you meet all requirements.

You've implemented the required functionality really well. The hover effects, responsiveness, and linking of album and artist URLs are all on point. You've split up components and passing props.

Remember, try to only pass along the data that is needed for the child component. For example the Aritist component only needs to know about the array of artists, so no need to pass along the whole album:

export const Artist = ({ album }) => {
  return (
    <div className="artists">
      {album.artists.map((artist, index) => {

Keep up the good work, And we will work more with different apporaches to svg's etc.

target="_blank"
rel="noopener noreferrer"
className="album-title"
aria-label={`Listen to ${album.name} on Spotify`}

Choose a reason for hiding this comment

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

⭐ nice to see that you're thinkning about a11y!

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