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

Add playlist editing #6184

Merged
merged 3 commits into from
Oct 13, 2024
Merged

Add playlist editing #6184

merged 3 commits into from
Oct 13, 2024

Conversation

thornbill
Copy link
Member

Changes

  • Updates the playlist editor to support editing playlists 😱

Currently this allows changing the playlist name and public access but not user level access controls

Issues
Part of #4698

@thornbill thornbill added the enhancement Improve existing functionality or small fixes label Oct 9, 2024
@thornbill thornbill added this to the v10.10.0 milestone Oct 9, 2024
@thornbill thornbill requested a review from a team as a code owner October 9, 2024 21:01
@solidsnake1298
Copy link
Member

I was able to toggle the public access setting of an existing playlist, which worked as expected.

Copy link
Member

@crobibero crobibero left a comment

Choose a reason for hiding this comment

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

Works for me, only note is that is uses the same icon as "Edit metadata"

Comment on lines +481 to +487
import('./playlisteditor/playlisteditor').then(({ default: PlaylistEditor }) => {
const playlistEditor = new PlaylistEditor();
playlistEditor.show({
id: itemId,
serverId
}).then(getResolveFunction(resolve, id, true), getResolveFunction(resolve, id));
});
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this more readable without chaining thens?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure I follow what you are suggesting 😅

@nielsvanvelzen
Copy link
Member

nielsvanvelzen commented Oct 13, 2024

Seems to work fine in my testing with a non-admin user. Only UX issue I could find is when you use a space as name it allows you to save that, not a major issue but might be good to check for that (e.g. name.trim().length).

I would also argue that the delete button should always be lower then the edit button.

@thornbill
Copy link
Member Author

The context menu needs a total overhaul so I'm not touching that here beyond adding the new entry 😅

@jellyfin-bot
Copy link
Collaborator

jellyfin-bot commented Oct 13, 2024

Cloudflare Pages deployment

Latest commit 3ad0fb0
Status ✅ Deployed!
Preview URL https://fb6ce114.jellyfin-web.pages.dev
Type 🔀 Preview

View build logs

Copy link
Contributor

@dmitrylyzo dmitrylyzo left a comment

Choose a reason for hiding this comment

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

LGTM. Just nitpicking.

Copy link

@thornbill thornbill merged commit 9c405e9 into jellyfin:master Oct 13, 2024
14 checks passed
@thornbill thornbill deleted the playlist-editor branch October 13, 2024 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improve existing functionality or small fixes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants