Skip to content

feat: implement embedded video player with ultra-large modal #770

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

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

feat: implement embedded video player with ultra-large modal #770

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
415b543
feat(videos): implement embedded video player with ultra-large modal
omsherikar Oct 23, 2025
ff36eaa
fix(videos): remove duplicate video thumbnail markup
omsherikar Oct 23, 2025
ba3daaf
fix(accessibility): improve focus restoration in video modal
omsherikar Oct 23, 2025
079128e
fix(accessibility): improve tab trapping and keyboard navigation
omsherikar Oct 23, 2025
fecd222
Merge branch 'main' into feature/embedded-video-player-620-clean
A1L13N Dec 15, 2025
6238960
Merge branch 'main' into feature/embedded-video-player-620-clean
A1L13N Dec 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
307 changes: 289 additions & 18 deletions web/templates/videos/list.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,125 @@
transform: translateY(0);
}
}

/* Video Modal - Following Tailwind Best Practices */
@layer components {
.video-modal {
@apply hidden fixed inset-0 z-50 bg-black bg-opacity-90 backdrop-blur-sm opacity-0 transition-all duration-300 ease-out;
}

.video-modal.show {
@apply flex items-center justify-center opacity-100;
}

.video-modal-content {
@apply relative bg-gradient-to-br from-gray-900 via-gray-800 to-gray-900 dark:from-gray-800 dark:via-gray-700 dark:to-gray-800 rounded-3xl overflow-hidden max-w-[95vw] w-full mx-2 shadow-2xl border border-white border-opacity-20 transform scale-95 transition-all duration-300 ease-out;
box-shadow:
0 32px 64px -12px rgba(0, 0, 0, 0.6),
0 0 0 1px rgba(255, 255, 255, 0.1),
inset 0 1px 0 rgba(255, 255, 255, 0.1);
}

.video-modal.show .video-modal-content {
@apply scale-100;
}

.video-modal-header {
@apply flex justify-between items-center px-6 py-5 bg-gradient-to-r from-gray-800 via-gray-700 to-gray-800 dark:from-gray-700 dark:via-gray-600 dark:to-gray-700 border-b border-gray-600 relative;
background: linear-gradient(135deg, #1f2937, #374151, #1f2937);
}

.video-modal-header::after {
content: '';
@apply absolute bottom-0 left-0 right-0 h-px bg-gradient-to-r from-transparent via-white to-transparent opacity-30;
}

.video-modal-title {
@apply text-white text-xl font-bold m-0 tracking-tight;
text-shadow: 0 2px 4px rgba(0, 0, 0, 0.5);
}

.video-modal-close {
@apply bg-white bg-opacity-10 border border-white border-opacity-20 text-white text-xl cursor-pointer p-2 rounded-lg transition-all duration-200 ease-out w-10 h-10 flex items-center justify-center hover:bg-opacity-20 hover:border-opacity-30 hover:scale-105 active:scale-95 focus:ring-2 focus:ring-offset-2 focus:ring-blue-500 focus:outline-none;
}

.video-modal-body {
@apply relative;
}

.video-modal-iframe {
@apply w-full h-[70vh] sm:h-[75vh] lg:h-[80vh] xl:h-[85vh] border-0 rounded-b-3xl;
background: linear-gradient(45deg, #1a1a1a, #2a2a2a);
}

.video-card {
@apply transition-all duration-300 ease-out relative overflow-hidden;
}

.video-card:hover {
@apply -translate-y-1 shadow-xl;
}

.video-card .video-link i.fa-play {
@apply transition-all duration-300 ease-out;
}

.video-card:hover .video-link i.fa-play {
@apply scale-110;
}

.sr-only {
@apply absolute w-px h-px p-0 -m-px overflow-hidden whitespace-nowrap border-0;
clip: rect(0, 0, 0, 0);
}
}

/* Enhanced Responsive Design - Mobile First */
@media (max-width: 768px) {
.video-modal-content {
@apply mx-1 rounded-2xl;
max-width: 98vw;
}

.video-modal-iframe {
@apply h-[65vh];
}

.video-modal-header {
@apply px-4 py-4;
}

.video-modal-title {
@apply text-lg;
}

.video-modal-close {
@apply w-9 h-9 p-1.5;
}
}

@media (max-width: 480px) {
.video-modal-content {
@apply mx-0.5 rounded-xl;
max-width: 99vw;
}

.video-modal-iframe {
@apply h-[60vh];
}

.video-modal-header {
@apply px-3 py-3;
}

.video-modal-title {
@apply text-base;
}

.video-modal-close {
@apply w-8 h-8 p-1;
}
}
Comment thread
omsherikar marked this conversation as resolved.
Comment thread
omsherikar marked this conversation as resolved.
</style>
{% endblock %}
{% block content %}
Expand Down Expand Up @@ -93,24 +212,13 @@
<!-- Video Preview -->
<div class="aspect-w-16 aspect-h-9 bg-gray-100 dark:bg-gray-700">
{% if video.thumbnail_url %}
<div class="relative aspect-w-16 aspect-h-9 overflow-hidden rounded-lg shadow-sm">
<div class="video-card relative aspect-w-16 aspect-h-9 overflow-hidden rounded-lg shadow-sm">
<img src="{{ video.thumbnail_url }}"
alt="{{ video.title }} thumbnail"
class="w-full h-full object-cover" />
<a href="{{ video.video_url }}"
target="_blank"
class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
<i class="fas fa-play text-white text-4xl"></i>
</a>
</div>
{% elif video.thumbnail_url %}
<div class="relative aspect-w-16 aspect-h-9 overflow-hidden rounded-lg shadow-sm">
<img src="{{ video.thumbnail_url }}"
alt="{{ video.title }} thumbnail"
class="w-full h-full object-cover" />
<a href="{{ video.video_url }}"
target="_blank"
class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
<a href="javascript:void(0)"
onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}', event)"
Comment on lines +219 to +220
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 15, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion | 🟠 Major

Use meaningful href with preventDefault instead of javascript:void(0).

Lines 219 and 234 use the outdated href="javascript:void(0)" pattern. This breaks progressive enhancement and accessibility:

  • Users cannot right-click → open in new tab
  • JavaScript failures leave non-functional links
  • Screen readers may not recognize them as proper links

Apply this diff to both links:

-                      <a href="javascript:void(0)"
+                      <a href="{{ video.video_url }}"
-                         onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')"
+                         onclick="event.preventDefault(); openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}', event)"
                         class="video-link absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">

And similarly for the title link:

-                    <a href="javascript:void(0)"
+                    <a href="{{ video.video_url }}"
-                       onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')"
+                       onclick="event.preventDefault(); openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}', event)"
                        class="video-link hover:text-orange-500 transition-colors cursor-pointer">{{ video.title }}</a>

Also applies to: 234-235

🤖 Prompt for AI Agents 
In web/templates/videos/list.html around lines 219-220 and 234-235, replace the
href="javascript:void(0)" pattern with a meaningful href that points to the
video URL (e.g. href="{{ video.video_url }}") and keep the onclick that opens
the modal, but update the onclick handler to accept the click event and call
event.preventDefault() inside the handler (openVideoModal(..., event)) so the
link remains a usable URL for right-click/open-in-new-tab and degrades
gracefully if JS is disabled; apply the same change to the title link and ensure
title is escaped as before.

class="video-link absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
<i class="fas fa-play text-white text-4xl"></i>
</a>
</div>
Expand All @@ -123,9 +231,9 @@
<!-- Video Info -->
<div class="p-4">
<h3 class="text-lg font-semibold mb-2 line-clamp-2">
<a href="{{ video.video_url }}"
target="_blank"
class="hover:text-orange-500 transition-colors">{{ video.title }}</a>
<a href="javascript:void(0)"
onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}', event)"
class="video-link hover:text-orange-500 transition-colors cursor-pointer">{{ video.title }}</a>
Comment on lines +234 to +236
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Oct 23, 2025

Choose a reason for hiding this comment

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

medium

Using href="javascript:void(0)" is an outdated practice. For better accessibility and progressive enhancement, it's recommended to provide a meaningful fallback URL in the href attribute. You could use the video URL itself as the href and then prevent the default link behavior in your onclick handler. This ensures that if JavaScript is disabled or fails to load, the link will still take the user to the video.

                    <a href="{{ video.video_url }}"
                       onclick="event.preventDefault(); openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')"
                       class="video-link hover:text-orange-500 transition-colors cursor-pointer">{{ video.title }}</a>

</h3>
<div class="flex items-center text-sm text-gray-500 dark:text-gray-400 mb-2">
{% if video.uploader %}
Expand Down Expand Up @@ -304,4 +412,167 @@
</div>
</div>
</main>

<!-- Video Modal - Semantic HTML5 Structure -->
<dialog id="videoModal" class="video-modal" role="dialog" aria-labelledby="videoModalTitle" aria-modal="true">
<div class="video-modal-content">
<header class="video-modal-header">
<h2 id="videoModalTitle" class="video-modal-title" tabindex="0"></h2>
<button id="videoModalClose" class="video-modal-close" aria-label="Close video player" type="button">
<span class="sr-only">Close</span>
<i class="fas fa-times" aria-hidden="true"></i>
</button>
</header>
<section class="video-modal-body">
<iframe id="videoModalIframe"
class="video-modal-iframe"
src=""
title="Video player"
frameborder="0"
allow="accelerometer; autoplay; clipboard-write; encrypted-media; gyroscope; picture-in-picture"
allowfullscreen>
</iframe>
</section>
</div>
</dialog>

<script>
// Get modal elements
const videoModal = document.getElementById('videoModal');
const videoModalTitle = document.getElementById('videoModalTitle');
const videoModalIframe = document.getElementById('videoModalIframe');
const videoModalClose = document.getElementById('videoModalClose');

// Store reference to the element that opened the modal for proper focus restoration
let triggeringElement = null;

// Function to extract YouTube video ID
function getYouTubeVideoId(url) {
const regExp = /^.*(youtu.be\/|v\/|u\/\w\/|embed\/|watch\?v=|&v=)([^#&?]*).*/;
const match = url.match(regExp);
return (match && match[2].length === 11) ? match[2] : null;
}

// Function to extract Vimeo video ID
function getVimeoVideoId(url) {
const regExp = /^.*(vimeo\.com\/)((channels\/[A-z]+\/)|(groups\/[A-z]+\/videos\/)|(videos\/)|)([0-9]+)/;

Check warning

Code scanning / CodeQL

Overly permissive regular expression range Medium

Suspicious character range that is equivalent to [A-Z[]^_`a-z].

Check warning

Code scanning / CodeQL

Overly permissive regular expression range Medium

Suspicious character range that is equivalent to [A-Z[]^_`a-z].
Comment on lines +457 to +458
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 15, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# Read the specific lines mentioned in the review
sed -n '450,460p' web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 587

🏁 Script executed:

# Check if there are any CodeQL warnings or similar validation rules in the repo
fd -e json -e yml -e yaml | grep -i "codeql\|lint\|eslint" | head -20

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 69

🏁 Script executed:

# Check the git history for when this code was added and if there are any related issues
git log --oneline -n 20 -- web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 159

🏁 Script executed:

# Check if there are other video ID validation functions or tests
rg "getVimeoVideoId|getYouTubeVideoId" -A 5 -B 2

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 1788

🏁 Script executed:

# Check if there are any other regex patterns that might have similar issues
rg "\[A-z\]" web/

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 228

Fix overly permissive regex character range [A-z] and enforce Vimeo ID validation.

The Vimeo URL regex at line 457–458 uses [A-z], which is overly permissive and includes unintended characters like [, \, ^, ` that fall between ASCII 'Z' (90) and 'a' (97).

Additionally, the regex should enforce a minimum of 8 digits for Vimeo video IDs to match the validation requirements:

    function getVimeoVideoId(url) {
-     const regExp = /^.*(vimeo\.com\/)((channels\/[A-z]+\/)|(groups\/[A-z]+\/videos\/)|(videos\/)|)([0-9]+)/;
+     const regExp = /^.*(vimeo\.com\/)((channels\/[A-Za-z]+\/)|(groups\/[A-Za-z]+\/videos\/)|(videos\/)|)([0-9]{8,})/;
      const match = url.match(regExp);
      return match ? match[6] : null;
    }
🧰 Tools
🪛 GitHub Check: CodeQL

[warning] 458-458: Overly permissive regular expression range
Suspicious character range that is equivalent to [A-Z\[\\]^_`a-z].

[warning] 458-458: Overly permissive regular expression range
Suspicious character range that is equivalent to [A-Z\[\\]^_`a-z].

🤖 Prompt for AI Agents 
In web/templates/videos/list.html around lines 457–458, the Vimeo URL regex is
overly permissive because it uses [A-z] and it doesn't enforce Vimeo ID length;
replace the character class with a proper alphabetic class (e.g. [A-Za-z]) for
channel/group names and change the video ID capture to require at least 8 digits
(e.g. use \d{8,}); keep the overall structure of the alternations but update the
pattern so it only matches valid Vimeo segments and returns the numeric ID when
present.

const match = url.match(regExp);
return match ? match[6] : null;
}

// Function to open video modal
function openVideoModal(videoUrl, videoTitle, event) {
// Capture the element that triggered the modal for proper focus restoration
triggeringElement = event?.target || document.activeElement;

let embedUrl = '';
const youtubeId = getYouTubeVideoId(videoUrl);
const vimeoId = getVimeoVideoId(videoUrl);

if (youtubeId) {
embedUrl = `https://www.youtube.com/embed/${youtubeId}?autoplay=1&rel=0&modestbranding=1&enablejsapi=1`;
} else if (vimeoId) {
embedUrl = `https://player.vimeo.com/video/${vimeoId}?autoplay=1&title=0&byline=0&portrait=0`;
} else {
// Fallback to original URL for unsupported platforms
window.open(videoUrl, '_blank');
return;
}

// Set title and accessibility attributes
videoModalTitle.textContent = videoTitle;
videoModalIframe.title = `Video player for ${videoTitle}`;

// Show modal with smooth animation
videoModal.style.display = 'flex';
videoModal.style.opacity = '0';
videoModal.querySelector('.video-modal-content').style.transform = 'scale(0.9) translateY(20px)';

// Trigger reflow and animate in
requestAnimationFrame(() => {
videoModal.classList.add('show');
videoModal.style.opacity = '1';
videoModal.querySelector('.video-modal-content').style.transform = 'scale(1) translateY(0)';
Comment on lines +487 to +495
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 15, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, check if the file exists and get its line count
wc -l web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 121

🏁 Script executed:

# Read the specific section (487-510) mentioned in the review
sed -n '480,520p' web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 1660

🏁 Script executed:

# Search for CSS class definitions related to video-modal
rg -A 5 "\.video-modal|\.show" web/templates/videos/list.html | head -100

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 3555

🏁 Script executed:

# Check for any <style> tags in the HTML file
grep -n "<style" web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 98

🏁 Script executed:

# Get lines around 512 to see the setTimeout
sed -n '500,520p' web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 900

🏁 Script executed:

# Also verify the exact structure of the openVideoModal function to understand the full context
sed -n '460,515p' web/templates/videos/list.html

Repository: alphaonelabs/alphaonelabs-education-website

Length of output: 2264

Remove inline style manipulation; use CSS class toggling for modal animations.

Lines 487–495 and 509–512 directly manipulate style.opacity and style.transform instead of relying on CSS transitions. This violates the coding guidelines ("Never use inline styles") and creates redundancy with CSS classes already defined for .video-modal.show and .video-modal-content.

Issues:

  • CSS defines transitions (duration-300, opacity-100, scale-100), but JavaScript overrides them with direct style assignments
  • Hardcoded setTimeout(..., 300) at line 512 depends on CSS duration; changing one breaks the other
  • Mixed concerns split styling between CSS and JavaScript

Fix: Use only CSS classes for animations. Remove inline style.opacity and style.transform manipulation:

function openVideoModal(videoUrl, videoTitle, event) {
  // ... existing code ...
  
  videoModal.style.display = 'flex';
  requestAnimationFrame(() => {
    videoModal.classList.add('show');
    document.body.style.overflow = 'hidden';
  });
  
  // ... rest of function ...
}

function closeVideoModal() {
  videoModal.classList.remove('show');
  
  setTimeout(() => {
    videoModal.style.display = 'none';
    videoModalIframe.src = '';
    document.body.style.overflow = '';
    if (triggeringElement && document.contains(triggeringElement)) {
      triggeringElement.focus();
    }
    triggeringElement = null;
  }, 300);
}

CSS already handles the rest via .video-modal.show and .video-modal.show .video-modal-content classes with proper transitions.

Also applies to: 509–512

🤖 Prompt for AI Agents 
web/templates/videos/list.html lines 487-495 and 509-512: the JS currently sets
inline style.opacity and style.transform and uses a hardcoded timeout tied to
CSS durations; remove all inline style manipulation and rely on CSS class
toggling for show/hide animations instead. Change openVideoModal to set display
(if needed) then requestAnimationFrame to add the "show" class and set
document.body overflow, and change closeVideoModal to remove the "show" class
and use the existing CSS transition duration to hide after the same timeout (or
better, use transitionend event) before clearing the iframe, resetting display,
restoring body overflow and returning focus to the triggering element. Ensure no
direct style.opacity/transform assignments remain and keep the timeout value in
sync with CSS or replace it with transitionend.

document.body.style.overflow = 'hidden';
});

// Set iframe source
videoModalIframe.src = embedUrl;

// Focus management for accessibility - focus title first for screen readers
videoModalTitle.focus();
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Dec 15, 2025

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Consider focus order in modal opening sequence.

Line 503 focuses on videoModalTitle when the modal opens. While this provides screen reader context, consider whether the close button (videoModalClose) might be a more intuitive first focus point for keyboard users.

Current approach (focus title first):

  • ✅ Screen readers announce title immediately
  • ⚠️ Long titles can be verbose

Alternative (focus close button):

  • ✅ Keyboard users can dismiss immediately
  • ✅ More standard modal pattern (close button as primary action)

This is not a blocker, but document the UX rationale if the current behavior is intentional.

🤖 Prompt for AI Agents 
In web/templates/videos/list.html around line 503, the modal currently calls
videoModalTitle.focus(); update the modal-opening logic to focus videoModalClose
instead (so keyboard users land on the close control first) and ensure the modal
still exposes its title to assistive tech (e.g., via aria-labelledby or
role="dialog"); alternatively, if you intend to keep focusing the title, add an
inline comment above line 503 documenting the UX rationale (screen reader
priority vs keyboard dismissal) so the choice is explicit for future reviewers.

}

// Function to close video modal
function closeVideoModal() {
// Animate out with smooth transition
videoModal.style.opacity = '0';
videoModal.querySelector('.video-modal-content').style.transform = 'scale(0.9) translateY(20px)';

setTimeout(() => {
// Close modal with proper accessibility
videoModal.classList.remove('show');
videoModal.style.display = 'none';
videoModalIframe.src = '';
document.body.style.overflow = '';

// Restore focus to the element that opened the modal
if (triggeringElement && document.contains(triggeringElement)) {
triggeringElement.focus();
}

// Clear the reference
triggeringElement = null;
}, 300);
}
Comment thread
omsherikar marked this conversation as resolved.

// Event listeners
videoModalClose.addEventListener('click', closeVideoModal);

// Close modal when clicking outside
videoModal.addEventListener('click', function(e) {
if (e.target === videoModal) {
closeVideoModal();
}
});

// Keyboard navigation
document.addEventListener('keydown', function(e) {
if (videoModal.classList.contains('show')) {
if (e.key === 'Escape') {
closeVideoModal();
return;
}

// Tab trapping for accessibility
if (e.key === 'Tab') {
const focusableElements = videoModal.querySelectorAll(
'button:not([disabled]), [href], input:not([disabled]), select:not([disabled]), textarea:not([disabled]), [tabindex]:not([tabindex="-1"])'
);

if (focusableElements.length === 0) {
// No focusable elements; prevent tab escape
e.preventDefault();
return;
}

const firstElement = focusableElements[0];
const lastElement = focusableElements[focusableElements.length - 1];
const activeEl = document.activeElement;

if (e.shiftKey) {
if (activeEl === firstElement) {
lastElement.focus();
e.preventDefault();
}
} else {
if (activeEl === lastElement) {
firstElement.focus();
e.preventDefault();
}
}
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}
});
</script>
{% endblock %}
Loading