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

[feat] Create "Add current track" button in playlist view #560

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

will-hut
Copy link

@will-hut will-hut commented Mar 27, 2024

I'm enjoying using Feishin as my music player for my library. I thought it would be nice to add a "Add current track" button to the playlist sidebar. That way, when a song is playing, I can quickly add it to as many playlists as I feel like it belongs in.

demo

Currently, I have it hiding the button if the playlist has rules (ie, a smart playlist). You can add a song to a shared playlist, but if you don't have (admin) permission, it throws an error toast.

This also allows adding duplicates of a song to a playlist, so a toggle somewhere to disable duplicates could be a nice feature.

I'm not a React dev and haven't really contributed to open source, so if there's any issues please let me know! I think I did a decent job, but I'm always looking for ways to make my code better.

fix dependency list

Add success toast
Copy link

vercel bot commented Mar 27, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
feishin ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 3, 2024 2:23am

@will-hut
Copy link
Author

This adds a feature similar to what is mentioned in #556

title: t('error.genericError', { postProcess: 'sentenceCase' }),
});
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should move the toast.success to onSuccess here

Copy link
Collaborator

@kgarner7 kgarner7 left a comment

Choose a reason for hiding this comment

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

Thanks for the commit! A few other thoughts:

  • This doesn't consider duplicates (e.g. adding the same song multiple times)
  • The new icon isn't quite intuitive, maybe it'd be worthwhile to remove the delay for tooltip (or another icon)

I'm also not sure if this is necessarily appropriate period, but I'll defer to @jeffvli for that.

const base = { defaultFullPlaylist, handlePlay: handlePlayPlaylist };
const base = {
defaultFullPlaylist,
handleAdd: handleAddCurrent,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be enabled for Navidrome shared playlists (only the owner can change them)

@kgarner7 kgarner7 requested a review from jeffvli April 3, 2024 02:19
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