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

feature: Autoplay the next video after completion of one #1308 #1434

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

SahilLamba0008
Copy link

PR Fixes:

  • 1 Autoplay Next Video: Implemented functionality to autoplay the next video after one completes.
  • 2 Minor Code Cleanup: Refactored some code related to the video player to ensure smooth transitions between videos.

Resolves #[Issue Number if there]

Screen.Recording.2024-10-05.at.5.50.26.PM.mp4

Checklist before requesting a review

  • [ ✅ ] I have performed a self-review of my code
  • [ ✅ ] I assure there is no similar/duplicate pull request regarding same issue

@devsargam
Copy link
Collaborator

Adding it to things to reivew

@devsargam
Copy link
Collaborator

Mean while if u can clear the conflicts @

@SahilLamba0008
Copy link
Author

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.

@Prashant-flick
Copy link
Contributor

Prashant-flick commented Oct 9, 2024

@SahilLamba0008
I think this will only work in the week in which the video is playing , it will not play the next week video, when you are at the last video of a week, can you fix that

@SahilLamba0008
Copy link
Author

@Prashant-flick @devsargam
Thank you for the feedback! While I understand the request to have videos automatically continue into the next week's module, I believe this approach may unnecessarily increase the load, especially since not all users may be ready to proceed to the next week's content right away.

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!

Screenshot 2024-10-09 at 3 53 46 PM

@SahilLamba0008
Copy link
Author

@Prashant-flick @devsargam Thank you for the feedback! While I understand the request to have videos automatically continue into the next week's module, I believe this approach may unnecessarily increase the load, especially since not all users may be ready to proceed to the next week's content right away.

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!

Screenshot 2024-10-09 at 3 53 46 PM

@devsargam should I create a issue for this and implement it ?

@devsargam
Copy link
Collaborator

@SahilLamba0008 whatever you are thinking is somewhat correct but I would need a bit more context
Can you share a loom https://www.loom.com/ with your explanation

@SahilLamba0008
Copy link
Author

@SahilLamba0008 whatever you are thinking is somewhat correct but I would need a bit more context Can you share a loom https://www.loom.com/ with your explanation

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 Up Next and Next Weeks Module Name with a play button.

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);
Copy link
Collaborator

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);
Copy link
Collaborator

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");
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Author

Choose a reason for hiding this comment

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

yes, on it.

@devsargam
Copy link
Collaborator

Also, as a follow up pr. Would you be interested to add next button in the video player itself

@SahilLamba0008
Copy link
Author

SahilLamba0008 commented Oct 25, 2024

Also, as a follow up pr. Would you be interested to add next button in the video player itself

Sure, will Raise a new PR by tomorrow EOD.

# Please enter a commit message to explain why this merge is necessary,
# especially if it merges an updated upstream into a topic branch.
#
# Lines starting with '#' will be ignored, and an empty message aborts
# the commit.
…-video-1308

# Conflicts:
#	pnpm-lock.yaml
#	src/components/Sidebar.tsx
#	src/components/VideoPlayer2.tsx
#	src/components/landing/footer.tsx
#	src/components/videoPlayer/icons.tsx
@SahilLamba0008
Copy link
Author

Also, as a follow up pr. Would you be interested to add next button in the video player itself

@devsargam can you please check if these are the intended changes.

  • I've made changes keeping in mind so that user can only navigate through contents of video type.
Screen.Recording.2024-10-25.at.8.26.17.PM.1.mp4

@SahilLamba0008
Copy link
Author

Also, as a follow up pr. Would you be interested to add next button in the video player itself

@devsargam can you please check if these are the intended changes.

  • I've made changes keeping in mind so that user can only navigate through contents of video type.

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.

@SahilLamba0008
Copy link
Author

@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');
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove

Copy link
Author

Choose a reason for hiding this comment

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

on it

Copy link
Author

Choose a reason for hiding this comment

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

done

Copy link
Author

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.

@SahilLamba0008
Copy link
Author

@devsargam Please review, I guess it's good now.

@SahilLamba0008
Copy link
Author

@devsargam Any updates on the review ?

@SahilLamba0008 SahilLamba0008 marked this pull request as draft November 7, 2024 12:20
@SahilLamba0008 SahilLamba0008 marked this pull request as ready for review November 7, 2024 12:20
@SahilLamba0008
Copy link
Author

hi @devsargam, should i open a new issue for this pr ?

@devsargam
Copy link
Collaborator

Let me review this one sir. Further more I will inform. I had to take a break so currently getting everything in line

@SahilLamba0008
Copy link
Author

SahilLamba0008 commented Nov 7, 2024

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.

@SahilLamba0008
Copy link
Author

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.

@devsargam
Copy link
Collaborator

I will review it today if possible
thanks for waiting

@devsargam
Copy link
Collaborator

image

@devsargam
Copy link
Collaborator

Works good on dev but breaks during the build might want to fix it @SahilLamba0008

@SahilLamba0008
Copy link
Author

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.

@devsargam
Copy link
Collaborator

Feature works yeah for a week it does but not after that which is fine. You did add the buttons so that should help us implement later on easily so yeah good work

@devsargam
Copy link
Collaborator

@SahilLamba0008 can you resolve them by today?

@SahilLamba0008
Copy link
Author

@SahilLamba0008 can you resolve them by today?

Yeah, already done. Will raise a PR when I reach home, probably by tonight.

@SahilLamba0008
Copy link
Author

@devsargam All set, bro. I've run the build command locally as well, just to be sure.

.env.example Outdated Show resolved Hide resolved
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.

feature: Autoplay the next video after completion of one
3 participants