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: playlist order fix: #1622 #1629

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

feat: playlist order fix: #1622 #1629

wants to merge 1 commit into from

Conversation

w3cj
Copy link
Member

@w3cj w3cj commented Mar 29, 2024

This addresses #1622

Introduced a PlaylistOrder enum to the Playlist model. I thought this might be useful because some playlists we might want to keep ordering exactly as it is on YouTube, some should be reversed and some should be sorted by video published asc / desc.

If this is too complex we can just go with order by Video Published desc

This PR:

  • Allows admins to set playlist sort from playlist admin page
  • Ordering type is used on playlists overview page and individual playlist page

What needs review:

  • This introduces extra queries when getting a playlist, since we have to get the preferred order type from the playlist table first before querying for included videos to set the orderBy property. Seems fine for just a few playlists, but when we have many, this could start to slow the page load down.
syntax-playlist-sort.mp4

Copy link

vercel bot commented Mar 29, 2024

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

Name Status Preview Updated (UTC)
syntax-website ✅ Ready (Inspect) Visit Preview Mar 29, 2024 3:56pm

export const load = async function ({ locals }) {
const playlists = await locals.prisma.playlist.findMany({
const playlistMeta = await locals.prisma.playlist.findMany({
Copy link
Member Author

Choose a reason for hiding this comment

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

This is one of the questionable queries. Have to get all playlists first, then get each playlist in a separate query.

});
const playlists = await Promise.all(
playlistMeta.map(async (playlist) => {
return locals.prisma.playlist.findFirst({
Copy link
Member Author

Choose a reason for hiding this comment

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

One query for every playlist... not ideal.

export const load = async function ({ locals, params }) {
const { p_slug } = params;
const playlist = await locals.prisma.playlist.findUnique({
const playlistMeta = await locals.prisma.playlist.findUnique({
Copy link
Member Author

Choose a reason for hiding this comment

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

Same thing here, but for a single playlist.

@w3cj w3cj requested a review from stolinski March 29, 2024 15:59
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.

1 participant