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

fix: artist page link sending to artist page #825

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

asartalo
Copy link

@asartalo asartalo commented Aug 27, 2024

Fixes #797.

Hello. This is my first contribution to the project so please let me know if there are any conventions that I should follow.

Copy link

netlify bot commented Aug 27, 2024

Deploy Preview for mirlo ready!

Name Link
🔨 Latest commit 3dab939
🔍 Latest deploy log https://app.netlify.com/sites/mirlo/deploys/66cd8114628dab0008fb36df
😎 Deploy Preview https://deploy-preview-825--mirlo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asartalo asartalo force-pushed the asartalo/artist-page-link-bug branch from 22afe7b to 3dab939 Compare August 27, 2024 07:32
@asartalo
Copy link
Author

Actually, this solution is not as simple as I thought it would be so I'll need more guidance. The changes I made in this PR affects a lot of other components external to the artist page since the component TrackGroupInfo component is used in other places. I'm thinking of closing this in favor of a simpler solution.

{trackGroup.artist?.name}
</Link>
)}
{<ArtistName artist={trackGroup.artist} />}
Copy link
Member

Choose a reason for hiding this comment

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

Do you have Prettier enabled for this project? This should automatically turn this into:

Suggested change
{<ArtistName artist={trackGroup.artist} />}
<ArtistName artist={trackGroup.artist} />

decluttering it a little bit. It's an outstanding project to add it to the repo by detault

Copy link
Member

@simonv3 simonv3 left a comment

Choose a reason for hiding this comment

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

lgtm! thank you!

Edit: ah I'm seeing your message now, let me look

Edit 2: Okay, it looks like TrackGroupInfo is kind of a misnomer, as it gets used for ArtistSquares as well (which is just the list of artists, in the same format as the trackgroups. A rename there might be in order (maybe something as simple as InfoUnderSquare?), but feel free to consider that out of the scope of this PR. I feel like a simple solution is to change the names of the classes you create eg. .track-group-name becomes .first-title and then adding that class name to the ArtistSquare

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.

Artist page link sending to artist page
2 participants