-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feature: Autoplay the next video after completion of one #1308 #1434
base: main
Are you sure you want to change the base?
feature: Autoplay the next video after completion of one #1308 #1434
Conversation
Adding it to things to reivew |
Mean while if u can clear the conflicts @ |
I've resolved the conflicts in the PR, Please review and let me know if there are any other changes needed. |
@SahilLamba0008 |
@Prashant-flick @devsargam Instead, I propose implementing a modal at the end of the last video in the current week's module, similar to YouTube's playlist behavior. This modal could prompt users with an option like "Continue to next week's videos" (refer to image for more context), giving them control over when they want to move forward. This would provide a smoother user experience and reduce potential server or client load by avoiding preloading content until the user chooses to proceed. Let me know if this sounds like a good compromise! |
@devsargam should I create a issue for this and implement it ? |
@SahilLamba0008 whatever you are thinking is somewhat correct but I would need a bit more context |
check this out ( https://www.loom.com/share/5be7066dc3ac4d81b1b1a7f7df72b7ae?sid=401e93af-557b-4e43-8c6c-1defd395dc84 ). I'm suggesting to add a modal just like udemy does, suggesting next weeks content on completion of the last video of current week. This modal can have text as |
src/db/course.ts
Outdated
@@ -163,6 +163,7 @@ export const getNextVideo = async (currentVideoId: number) => { | |||
}, | |||
}); | |||
cache.set('getNextVideo', [currentVideoId.toString()], latestContent); | |||
// console.log("---\nCourse.ts :", latestContent); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a lot of console.log comments. That's really bad. Could you please resolve those?
parts.push(nextContent.id.toString()); | ||
|
||
const newPath = parts.join('/'); | ||
// console.log('\n Next Content :', nextContent, newPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here
@@ -71,6 +71,21 @@ export const ContentRendererClient = ({ | |||
setShowChapters((prev) => !prev); | |||
}; | |||
|
|||
const toggleNextVideo = async () => { | |||
// console.log("Video Completed : ContentRendererClient.tsx"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, on it.
Also, as a follow up pr. Would you be interested to add |
Sure, will Raise a new PR by tomorrow EOD. |
…-video-1308 # Conflicts: # pnpm-lock.yaml # src/components/Sidebar.tsx # src/components/VideoPlayer2.tsx # src/components/landing/footer.tsx # src/components/videoPlayer/icons.tsx
495906b
to
f13059f
Compare
@devsargam can you please check if these are the intended changes.
Screen.Recording.2024-10-25.at.8.26.17.PM.1.mp4 |
@devsargam changes are made in this commit "Added navigation buttons within videojs controller with autoplay feature" please review. |
@devsargam Is this okay or do I've to raise a new issue for this. |
prisma/seed.ts
Outdated
@@ -181,6 +181,7 @@ async function seedNotionMetadata() { | |||
|
|||
async function seedVideoMetadata() { | |||
try { | |||
console.log('seeding video meta data'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there are several other consoles which are not added by me, I can remove them as well if you want.
@devsargam Please review, I guess it's good now. |
@devsargam Any updates on the review ? |
hi @devsargam, should i open a new issue for this pr ? |
Let me review this one sir. Further more I will inform. I had to take a break so currently getting everything in line |
Apologies sir! Please take your time. I didn't mean to rush you. |
Hi @devsargam, I've resolved the latest conflicts. Still waiting for your review—please take a look whenever you're free. I'm up for any further tweaks required. |
I will review it today if possible |
Works good on dev but breaks during the build might want to fix it @SahilLamba0008 |
@devsargam I assume the feature is working as expected—please confirm. I'll address the build error ASAP. |
PR Fixes:
Resolves #[Issue Number if there]
Screen.Recording.2024-10-05.at.5.50.26.PM.mp4
Checklist before requesting a review