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.

Sign up for GitHub

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

Jump to bottom

✨(video_player) lazy load embed video player #2171

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

✨(video_player) lazy load embed video player #2171

wants to merge 1 commit into from

Conversation

igobranco
Copy link
Collaborator

Instead of loading the external video player,
it only loads the video player if the user clicks on the big ▶ icon.

It only works on the published page. On the edit mode it still show the old behavior.
This has performance benefits so the external video player JS libraries, like Youtube, only loads when the user wants to view the video.

The image that is uses is the video poster or the course cover.

image

@igobranco igobranco self-assigned this Dec 8, 2023
@igobranco igobranco force-pushed the igobranco/lazy-load-video-player branch from fd1091b to 9ab3da6 Compare December 8, 2023 23:23
@jbpenrath jbpenrath self-requested a review December 11, 2023 15:11
jbpenrath
jbpenrath reviewed Dec 11, 2023
View reviewed changes
Copy link
Member

@jbpenrath jbpenrath left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Currently, the user has to click twice to start the video that is weird but unfortunately autoplay has several restriction to be enable (add allow="autoplay" to the iframe and the video has to be muted by default...)

CleanShot.2023-12-11.at.23.47.18.mp4

src/richie/apps/core/templates/djangocms_video/default/video_player.html Outdated Show resolved Hide resolved
src/richie/apps/core/templates/djangocms_video/default/video_player.html Outdated Show resolved Hide resolved
tests/apps/core/test_videoplayer.py Outdated Show resolved Hide resolved
@igobranco igobranco force-pushed the igobranco/lazy-load-video-player branch 4 times, most recently from d1f0949 to 1e9bd87 Compare December 2, 2024 23:30
@igobranco igobranco force-pushed the igobranco/lazy-load-video-player branch from 1e9bd87 to ea91b1a Compare December 12, 2024 13:58
@igobranco igobranco force-pushed the igobranco/lazy-load-video-player branch 4 times, most recently from 6f5fc21 to 2cca752 Compare January 24, 2025 15:31
@igobranco
Copy link
Collaborator Author

Added a feature toggle, using the RICHIE_VIDEO_LAZY_LOAD Django setting.
To use the new lazy implementation or use the older/existing one. By default use the older/existing existing implementation.
So by default merging this PR won't change anything on Richie site.

I refactor the implementation.
Instead of reusing the player iframe and changing its content, use a simpler JS code to hide the play icon and show the iframe video player.

This implementation change, from previous review allows to fix the problem of the "back button", after the user clicks on play and wants to go back to previous page, the page was only removing the Youtube player with the play icon.

@igobranco
✨(video_player) add lazy load video player option
964b360 
Add an option to lazy load iframe video player.
Instead of always loading the external video player,
it only loads the video player if the user clicks on the
big play ▶ icon.
@igobranco igobranco force-pushed the igobranco/lazy-load-video-player branch from 2cca752 to 964b360 Compare January 24, 2025 16:00
@igobranco
Copy link
Collaborator Author

@sandroscosta Did we need to change something about accessibility?

@igobranco
Copy link
Collaborator Author

@sandroscosta can you check if we need to include something about accessibility?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbpenrath jbpenrath jbpenrath left review comments

@sandroscosta sandroscosta Awaiting requested review from sandroscosta

At least 1 approving review is required to merge this pull request.

Assignees

@igobranco igobranco

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@igobranco @jbpenrath