feat: implement embedded video player with ultra-large modal#770
feat: implement embedded video player with ultra-large modal#770omsherikar wants to merge 6 commits into
Conversation
- Add beautiful video modal with YouTube/Vimeo embed support - Implement ultra-large responsive modal (95vw width, 70-85vh height) - Add smooth animations and loading states with gradient backgrounds - Include full accessibility support with ARIA labels and focus management - Use semantic HTML5 elements (dialog, header, section) for better structure - Follow Tailwind CSS best practices with @layer components - Add keyboard navigation and screen reader support - Implement mobile-first responsive design with dark mode support - Replace YouTube redirects with embedded video player - Maintain CONTRIBUTING.md compliance throughout Closes alphaonelabs#620
Summary of ChangesHello @omsherikar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the user experience for viewing educational videos by integrating an embedded video player within an ultra-large, responsive modal. Instead of navigating away from the page, users can now watch videos directly, benefiting from a seamless, modern interface and improved accessibility features. The changes include comprehensive styling, dynamic video embedding logic for popular platforms like YouTube and Vimeo, and thoughtful keyboard and screen reader support. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughA single template file is modified to add an inline Tailwind-styled video modal and associated JS. Video links now open an in-page dialog that embeds YouTube/Vimeo iframes (autoplay), manages ARIA and focus, supports Escape/outside-click dismissal, and includes responsive styling and animations. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Page as Videos List Page (HTML)
participant JS as Modal Script
participant Modal as Dialog UI
participant IFrame as Embed IFrame
User->>Page: Click video thumbnail/title (modal-trigger link)
Page->>JS: call openVideoModal(videoUrl, videoTitle, event)
activate JS
JS->>JS: parse URL -> identify YouTube or Vimeo + media ID
JS->>JS: build embed URL with autoplay params
JS->>Modal: set title, ARIA attributes, show/animate dialog
JS->>IFrame: set iframe.src to embed URL
JS->>JS: trap focus inside Modal, store trigger element
deactivate JS
rect rgb(230,245,255)
Note over User,Modal: Modal active — responds to Escape, outside-click, close button, Tab trap
end
User->>Modal: Request close (Escape / outside click / close button)
activate JS
JS->>Modal: animate close, remove ARIA attributes
JS->>IFrame: clear iframe.src
JS->>JS: restore focus to trigger and page scroll
deactivate JS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an embedded video player within a modal, which is a great enhancement for user experience. The implementation correctly uses a <dialog> element for accessibility and includes features like keyboard navigation and focus trapping. However, there are several areas for improvement. The CSS is currently embedded within the HTML, which should be moved to a separate file for better maintainability. The JavaScript for animations is a bit brittle, relying on direct style manipulation and setTimeout instead of more robust class-toggling and event-based approaches. There's also a flaw in the focus restoration logic upon closing the modal, which impacts accessibility. Additionally, I've pointed out a redundant code block and suggested modernizing the anchor tags for opening the modal. My review includes specific suggestions to address these points.
| <a href="javascript:void(0)" | ||
| onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')" | ||
| class="video-link absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity"> |
There was a problem hiding this comment.
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 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 }}')" | ||
| class="video-link hover:text-orange-500 transition-colors cursor-pointer">{{ video.title }}</a> |
There was a problem hiding this comment.
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>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
web/templates/videos/list.html (2)
8-144: Move custom CSS to dedicated stylesheet or use Tailwind extensions; don't embed styling in templates.The coding guidelines explicitly require "Use Tailwind CSS classes only in templates (no custom CSS or inline styles)." This 136-line
<style>block violates that requirement. Custom@layer components,@keyframes,box-shadowdefinitions, and media queries belong in a dedicated CSS file where they can be:
- Properly formatted with djlint
- Reused across templates
- Easier to maintain and test
- Bundled/minified in production
Extract all custom styles to a new stylesheet (e.g.,
web/static/css/video-modal.css), then import it in the template's{% block extra_head %}or in your main CSS pipeline.Here's the recommended structure:
/* web/static/css/video-modal.css */ @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; } /* ... rest of component styles ... */ } @keyframes fadeIn { from { opacity: 0; transform: translateY(-10px); } to { opacity: 1; transform: translateY(0); } } .form-section { display: block; animation: fadeIn 0.3s; } @media (max-width: 768px) { /* ... responsive styles ... */ }Then in the template:
{% block extra_head %} <link rel="stylesheet" href="{% static 'css/video-modal.css' %}"> {% endblock %}This also allows you to leverage Tailwind's
@applyand responsive utilities properly.
219-247: Use semantic link markup or<button>elements instead ofhref="javascript:void(0)"; improves accessibility and keyboard navigation.Using
href="javascript:void(0)"withonclickhandlers breaks keyboard semantics, screen reader announcements, and tab order predictability. The video title link (line 245) and thumbnail play buttons (lines 219, 231) should be actual<button>elements or proper links withhrefattributes that defer to JavaScript event handlers via proper listeners.Refactor to use semantic
<button>elements:- <a href="javascript:void(0)" + <button type="button" - onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')" + data-video-url="{{ video.video_url|escapejs }}" + data-video-title="{{ video.title|escapejs }}" + aria-label="Play video: {{ video.title }}" 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> + </button>And for the title link:
- <a href="javascript:void(0)" + <button type="button" - onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}')" + data-video-url="{{ video.video_url|escapejs }}" + data-video-title="{{ video.title|escapejs }}" class="video-link hover:text-orange-500 transition-colors cursor-pointer">{{ video.title }}</button>Then add a single event delegation listener in the JavaScript:
document.addEventListener('click', function(e) { const btn = e.target.closest('button.video-link'); if (btn) { openVideoModal(btn.dataset.videoUrl, btn.dataset.videoTitle); } });This approach:
- Maintains proper keyboard focus and tab order
- Works reliably with screen readers
- Avoids the
javascript:protocol antipattern- Centralizes event handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
web/templates/videos/list.html(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.cursorrules)
**/*.html: Always use Tailwind CSS utility classes for styling HTML elements
Never use inline styles on elements (no style="...")
Use the project color scheme via Tailwind classes: teal-300 (primary), gray-600 (secondary), green-600 (success), yellow-600 (warning), red-600 (danger), with dark: variants
Containers should use: container mx-auto px-4
Cards should use: bg-white dark:bg-gray-800 rounded-lg shadow-lg
Form inputs should use: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500
Form labels should use: block text-sm font-medium text-gray-700 dark:text-gray-300
Primary buttons should use: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200
Secondary buttons should use: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300
Headings use Tailwind typography: text-2xl font-bold (adjust size as needed)
Body text should use: text-gray-600 dark:text-gray-300
Links should use: text-blue-600 hover:text-blue-800 dark:text-blue-400
Use flex and grid utilities for layouts instead of custom CSS
Use consistent spacing with gap-{size} and space-{x|y}-{size} utilities
Ensure responsive design using Tailwind responsive prefixes (sm:, md:, lg:, xl:)
Always include dark mode alternatives using the dark: prefix where appropriate
Use dark:bg-gray-800 for card backgrounds in dark mode
Use dark:text-gray-300 for text in dark mode
Maintain proper heading hierarchy (h1 > h2 > h3...)
Include appropriate ARIA labels and roles where needed
Ensure sufficient color contrast between text and background
Add visible hover and focus states for all interactive elements
Files:
web/templates/videos/list.html
web/templates/**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
web/templates/**/*.html: Format HTML/Django templates with djlint using 120-character line length
Use Tailwind CSS classes only in templates (no custom CSS or inline styles)
Include dark mode variants using the "dark:" prefix in templates
Ensure proper HTML structure and accessibility in templates
Follow project color scheme in templates (teal-300, gray-600, green-600, yellow-600, red-600)
Files:
web/templates/videos/list.html
🔇 Additional comments (1)
web/templates/videos/list.html (1)
427-448: Good semantic HTML5 structure; dialog element and accessibility attributes are well-implemented.The dialog element (line 428) includes:
- Proper
role="dialog"andaria-modal="true"attributesaria-labelledbypointing to the title- Semantic
<header>,<section>elements- Descriptive iframe
titleattribute- Screen-reader-only text for icon-only buttons
This is a solid foundation for accessible modal implementation.
- Remove redundant {% elif video.thumbnail_url %} block
- Fix copy-paste error that created identical conditional blocks
- Clean up template structure for better maintainability
The second condition was unreachable since the first already handled
the video.thumbnail_url case.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
web/templates/videos/list.html (4)
219-221: Replacehref="javascript:void(0)"with semantic href and event prevention.Using
javascript:void(0)is outdated and breaks progressive enhancement. If JavaScript fails to load, users cannot access the video. Use the actual video URL ashrefand prevent default navigation in the click handler.Apply this diff to both video link locations:
- <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 }}')"Also applies to: 234-236
481-504: Avoid direct inline style manipulation; use class-based CSS transitions instead.Lines 481–489 and 503–504 directly manipulate
style.display,style.opacity, andstyle.transform. This:
- Duplicates styling logic already in CSS (
.showclass,.video-modal-contenttransitions)- Makes animations brittle—CSS changes won't sync with JavaScript timeouts
- Conflicts with Tailwind's responsive/dark-mode utilities
- Violates separation of concerns
Refactored approach: control visibility and animation via class toggling only. Update CSS to handle all visual state transitions:
.video-modal { @apply hidden fixed inset-0 z-50 bg-black/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 rounded-3xl overflow-hidden max-w-[95vw] w-full mx-2 shadow-2xl border border-white/20 scale-95 transition-all duration-300 ease-out; } .video-modal.show .video-modal-content { @apply scale-100; }Then simplify the JavaScript:
function openVideoModal(videoUrl, videoTitle) { // ... extract IDs, build embedUrl ... videoModalTitle.textContent = videoTitle; videoModalIframe.title = `Video player for ${videoTitle}`; videoModalIframe.src = embedUrl; // Show modal via class only (CSS handles display, opacity, transform) videoModal.style.display = 'flex'; requestAnimationFrame(() => { videoModal.classList.add('show'); document.body.style.overflow = 'hidden'; videoModalClose.focus(); }); } function closeVideoModal() { videoModal.classList.remove('show'); // Use transitionend or setTimeout to wait for CSS animation setTimeout(() => { videoModal.style.display = 'none'; videoModalIframe.src = ''; document.body.style.overflow = ''; }, 300); // Match .video-modal transition-duration-300 }
513-517: Store and restore the triggering element for reliable focus management.Line 514 checks
document.activeElement.classList.contains('video-link'), but this is unreliable:
- Focus moves to the close button immediately after the modal opens (line 497:
videoModalClose.focus())- When the modal closes,
document.activeElementis likely the close button or somewhere else, not the link that opened the modal- Screen readers may not announce the focus change correctly
Store a reference to the triggering element and restore it explicitly:
let triggeringElement = null; function openVideoModal(videoUrl, videoTitle) { triggeringElement = document.activeElement; // Capture the element that triggered the modal // ... rest of modal opening logic ... videoModalClose.focus(); } function closeVideoModal() { videoModal.classList.remove('show'); setTimeout(() => { 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(); } }, 300); }
538-554: Improve tab-trapping selector to exclude disabled elements and handle edge cases.Line 539 selects focusable elements but does not exclude disabled form controls or account for elements with
tabindex:const focusableElements = videoModal.querySelectorAll('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])');This can trap focus on disabled buttons or miss valid focusable elements. Additionally, if no focusable elements exist,
firstElementandlastElementwill beundefined, causing runtime errors.Refactored selector:
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 from escaping 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(); } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
web/templates/videos/list.html(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.html
📄 CodeRabbit inference engine (.cursorrules)
**/*.html: Always use Tailwind CSS utility classes for styling HTML elements
Never use inline styles on elements (no style="...")
Use the project color scheme via Tailwind classes: teal-300 (primary), gray-600 (secondary), green-600 (success), yellow-600 (warning), red-600 (danger), with dark: variants
Containers should use: container mx-auto px-4
Cards should use: bg-white dark:bg-gray-800 rounded-lg shadow-lg
Form inputs should use: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500
Form labels should use: block text-sm font-medium text-gray-700 dark:text-gray-300
Primary buttons should use: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200
Secondary buttons should use: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300
Headings use Tailwind typography: text-2xl font-bold (adjust size as needed)
Body text should use: text-gray-600 dark:text-gray-300
Links should use: text-blue-600 hover:text-blue-800 dark:text-blue-400
Use flex and grid utilities for layouts instead of custom CSS
Use consistent spacing with gap-{size} and space-{x|y}-{size} utilities
Ensure responsive design using Tailwind responsive prefixes (sm:, md:, lg:, xl:)
Always include dark mode alternatives using the dark: prefix where appropriate
Use dark:bg-gray-800 for card backgrounds in dark mode
Use dark:text-gray-300 for text in dark mode
Maintain proper heading hierarchy (h1 > h2 > h3...)
Include appropriate ARIA labels and roles where needed
Ensure sufficient color contrast between text and background
Add visible hover and focus states for all interactive elements
Files:
web/templates/videos/list.html
web/templates/**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
web/templates/**/*.html: Format HTML/Django templates with djlint using 120-character line length
Use Tailwind CSS classes only in templates (no custom CSS or inline styles)
Include dark mode variants using the "dark:" prefix in templates
Ensure proper HTML structure and accessibility in templates
Follow project color scheme in templates (teal-300, gray-600, green-600, yellow-600, red-600)
Files:
web/templates/videos/list.html
- Store reference to triggering element for reliable focus restoration - Pass event object to openVideoModal function to capture caller - Replace unreliable focus detection with explicit element reference - Add proper cleanup of triggering element reference - Ensure focus returns to the exact element that opened the modal - Improve screen reader experience and keyboard navigation Fixes focus restoration issues where focus could shift during modal interaction, making the close button the active element instead of the original triggering link.
- Enhance tab trapping logic to handle edge cases properly - Add disabled element filtering to focusable elements selector - Handle case where no focusable elements exist (prevent tab escape) - Improve focus order by focusing title first for screen readers - Add tabindex to modal title for better keyboard accessibility - Ensure robust tab cycling between first and last elements - Fix potential issues with iframe focusability The improved implementation now properly handles: - Disabled form elements (excluded from tab order) - Empty focusable elements list (prevents tab escape) - Better focus order (title -> close button) - More robust element selection criteria
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| // 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
|
|
||
| // 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
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
web/templates/videos/list.html (1)
26-143: Remove non-Tailwind CSS properties and refactor to pure Tailwind utilities.The CSS block uses non-Tailwind properties (
box-shadow,text-shadow,linear-gradient,clip) mixed with@applydirectives, which violates the guideline requiring "Tailwind CSS classes only in templates (no custom CSS or inline styles)." While@applyis a Tailwind feature, adding custom CSS properties alongside it defeats the purpose of the utility-first approach.Refactor by either:
- Replacing
box-shadowandtext-shadowvalues with Tailwind shadow utilities (shadow-*,drop-shadow-*)- Converting
linear-gradientoverrides to Tailwind gradient utilities (bg-gradient-to-*)- Replacing
clip: rect()with Tailwind's built-insr-onlyutilityThis aligns the code with the project's stated guidelines and improves maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
web/templates/videos/list.html(4 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
web/templates/**/*.html
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
web/templates/**/*.html: Format HTML/Django templates with djlint using 120-character line length
Use Tailwind CSS classes only in templates (no custom CSS or inline styles)
Include dark mode variants using the "dark:" prefix in templates
Ensure proper HTML structure and accessibility in templates
Follow project color scheme in templates (teal-300, gray-600, green-600, yellow-600, red-600)
Files:
web/templates/videos/list.html
**/*.{html,htm}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{html,htm}: Always use Tailwind CSS classes for styling HTML elements
Never use inline styles
Follow the project's color scheme using Tailwind's color classes (Primary: teal-300, Secondary: gray-600, Success: green-600, Warning: yellow-600, Danger: red-600, Dark mode variants using dark: prefix)
Use Tailwind container classes: container mx-auto px-4 for containers
Use Tailwind card classes: bg-white dark:bg-gray-800 rounded-lg shadow-lg for cards
Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs
Use Tailwind form label classes: block text-sm font-medium text-gray-700 dark:text-gray-300 for form labels
Use Tailwind primary button classes: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200
Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300
Use Tailwind heading classes: text-2xl font-bold (adjust size as needed) for headings
Use Tailwind body text classes: text-gray-600 dark:text-gray-300 for body text
Use Tailwind link classes: text-blue-600 hover:text-blue-800 dark:text-blue-400 for links
Use flex and grid for layouts
Use consistent spacing with gap-{size} and space-{y|x}-{size} Tailwind classes
Use responsive design with Tailwind prefixes: sm:, md:, lg:, xl:
Always include dark mode alternatives using dark: prefix for elements
Use dark:bg-gray-800 for dark mode card backgrounds
Use dark:text-gray-300 for dark mode text
Use proper heading hierarchy for accessibility
Include proper ARIA labels where needed for accessibility
Ensure sufficient color contrast for accessibility
Add hover/focus states for interactive elements
Files:
web/templates/videos/list.html
**/*.{html,htm,css}
📄 CodeRabbit inference engine (AGENTS.md)
Never use custom CSS classes
Files:
web/templates/videos/list.html
🧠 Learnings (9)
📓 Common learnings
Learnt from: Abhishek-Punhani
Repo: alphaonelabs/alphaonelabs-education-website PR: 337
File: web/templates/courses/detail.html:1506-1560
Timestamp: 2025-04-07T11:33:41.277Z
Learning: For the alphaonelabs-education-website project, the team prefers to keep PRs focused and manageable in size. Additional enhancements like accessibility improvements should be addressed in separate follow-up issues rather than expanding the scope of an existing PR.
📚 Learning: 2025-04-05T16:36:48.539Z
Learnt from: Legacywave1
Repo: alphaonelabs/alphaonelabs-education-website PR: 457
File: web/templates/base.html:149-357
Timestamp: 2025-04-05T16:36:48.539Z
Learning: In the alphaonelabs-education-website project, avoid using nested CSS syntax like `.dark & {}` which is only valid in preprocessors like SCSS. Instead, use standard CSS selectors like `.dark .element-class {}` since the website appears to use standard CSS.
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Use Tailwind primary button classes: bg-teal-300 hover:bg-teal-400 text-white px-6 py-2 rounded-lg transition duration-200
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Use Tailwind secondary button classes: bg-gray-100 hover:bg-gray-200 dark:bg-gray-700 text-gray-700 dark:text-gray-300
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-10-05T06:35:47.159Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-05T06:35:47.159Z
Learning: Applies to web/templates/**/*.html : Use Tailwind CSS classes only in templates (no custom CSS or inline styles)
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Follow the project's color scheme using Tailwind's color classes (Primary: teal-300, Secondary: gray-600, Success: green-600, Warning: yellow-600, Danger: red-600, Dark mode variants using dark: prefix)
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Use Tailwind card classes: bg-white dark:bg-gray-800 rounded-lg shadow-lg for cards
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Use Tailwind link classes: text-blue-600 hover:text-blue-800 dark:text-blue-400 for links
Applied to files:
web/templates/videos/list.html
📚 Learning: 2025-12-15T00:22:20.785Z
Learnt from: CR
Repo: alphaonelabs/alphaonelabs-education-website PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-15T00:22:20.785Z
Learning: Applies to **/*.{html,htm} : Use Tailwind form input classes: w-full px-4 py-2 border border-gray-300 dark:border-gray-600 rounded-lg focus:ring-2 focus:ring-blue-500 for form inputs
Applied to files:
web/templates/videos/list.html
🪛 GitHub Check: CodeQL
web/templates/videos/list.html
[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].
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
web/templates/videos/list.html (2)
416-437: Semantic HTML5 modal structure with strong accessibility foundation.The modal markup uses proper semantic HTML (
<dialog>element,<header>,<section>) with comprehensive ARIA attributes (role="dialog",aria-modal="true",aria-labelledby). The close button includes properaria-labelandsr-onlytext. The iframe includes a title attribute and appropriateallowpermissions.This foundation is solid and aligns with accessibility best practices.
548-574: Tab trapping implementation is complete and handles edge cases.The keyboard navigation logic properly:
- Selects focusable elements with appropriate exclusions (disabled, tabindex="-1")
- Handles empty focusable list by preventing Tab escape
- Cycles focus between first and last elements for both forward and backward Tab
- Uses
document.activeElementto track current focus contextThis implementation aligns with WCAG focus management best practices.
| <a href="javascript:void(0)" | ||
| onclick="openVideoModal('{{ video.video_url }}', '{{ video.title|escapejs }}', event)" |
There was a problem hiding this comment.
🛠️ 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.
| function getVimeoVideoId(url) { | ||
| const regExp = /^.*(vimeo\.com\/)((channels\/[A-z]+\/)|(groups\/[A-z]+\/videos\/)|(videos\/)|)([0-9]+)/; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the specific lines mentioned in the review
sed -n '450,460p' web/templates/videos/list.htmlRepository: 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 -20Repository: 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.htmlRepository: 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 2Repository: 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.
| 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)'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and get its line count
wc -l web/templates/videos/list.htmlRepository: 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.htmlRepository: 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 -100Repository: 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.htmlRepository: 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.htmlRepository: 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.htmlRepository: 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.
| videoModalIframe.src = embedUrl; | ||
|
|
||
| // Focus management for accessibility - focus title first for screen readers | ||
| videoModalTitle.focus(); |
There was a problem hiding this comment.
🧹 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.
Related issues
Fixes #620
Checklist
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.