Open
Conversation
Contributor
👀 Peer Review RequiredHi @Shrey1006! This pull request does not yet have a peer review. Before this PR can be merged, please request a review from one of your peers:
Thank you for contributing! 🎉 |
Contributor
WalkthroughAdds an internal video detail page: new URL pattern and view to fetch a video by id, a new template that embeds YouTube videos and shows metadata, and updates to the videos list template to link to the new detail route. Note: Changes
Sequence Diagram(s)sequenceDiagram
participant User as Client (browser)
participant List as Videos List Page
participant Detail as Django view: video_detail
participant DB as Database (EducationalVideo)
participant YT as YouTube (embed)
rect rgba(200,200,255,0.5)
User->>List: click video link
end
rect rgba(200,255,200,0.5)
List->>Detail: GET /videos/{id}/
Detail->>DB: SELECT * FROM EducationalVideo WHERE id={id}
DB-->>Detail: video record
Detail-->>User: render video_detail HTML (includes iframe src=https://www.youtube.com/embed/{video_id}?rel=0&modestbranding=1)
end
rect rgba(255,200,200,0.5)
User->>YT: Browser requests iframe src (YouTube embed)
YT-->>User: video stream/content
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
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)
105-113:⚠️ Potential issue | 🟡 MinorRemove or fix the unreachable duplicate thumbnail branch.
{% elif video.thumbnail_url %}can never execute because the previous{% if video.thumbnail_url %}already captures it. This leaves dead code and retains staletarget="_blank"behavior in that dead path.Suggested fix
-{% 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="{% url 'video_detail' video.id %}" - 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>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/list.html` around lines 105 - 113, The template contains a redundant `{% elif video.thumbnail_url %}` branch that can never run because an earlier `{% if video.thumbnail_url %}` already handles the same condition; remove the unreachable `{% elif video.thumbnail_url %}` block (the entire duplicate div/img/a markup) and, if any behavior from that branch needs to be preserved, merge it into the original `{% if video.thumbnail_url %}` branch (e.g., adjust the anchor/attributes there) — specifically inspect and consolidate the anchor element that currently includes target="_blank" so you either remove that attribute or ensure it's applied consistently in the remaining `if video.thumbnail_url` markup.
124-127:⚠️ Potential issue | 🟠 MajorKeep title clicks inside the internal video-detail flow.
The card title still links directly to
video.video_urlin a new tab, so users can still be sent to YouTube. That contradicts the PR objective.Suggested fix
-<a href="{{ video.video_url }}" - target="_blank" - class="hover:text-orange-500 transition-colors">{{ video.title }}</a> +<a href="{% url 'video_detail' video.id %}" + class="hover:text-orange-500 transition-colors">{{ video.title }}</a>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/list.html` around lines 124 - 127, The title link currently points to external video.video_url and opens in a new tab (href="{{ video.video_url }}" and target="_blank"), which bypasses the internal video-detail flow; change the anchor to point to the internal video detail route (e.g., use the app's route helper or an internal path that references video.id or video.slug such as the video_detail view) and remove target="_blank" so clicks stay within the app and follow the internal video-detail flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/videos/list.html`:
- Around line 100-103: The icon-only play link (anchor with href "{% url
'video_detail' video.id %}" containing the <i class="fas fa-play ...">) needs an
accessible name: add an aria-label (e.g., aria-label="Play {{ video.title }}" or
a generic "Play video") or include visually-hidden text inside the anchor so
screen readers receive a meaningful label; update the anchor element near the
play icon accordingly to preserve visual appearance while providing the ARIA
name.
In `@web/templates/videos/video_detail.html`:
- Around line 6-43: The template currently never uses the related_videos context
variable built in the view; either remove the DB query or render them — to fix,
add a related-videos section in video_detail.html after the description that
checks for related_videos and iterates over the related_videos list (variable
name: related_videos) to render each item's thumbnail, title and link to its
detail (use video.id or video.slug as used elsewhere), or if you prefer to avoid
rendering, delete the related_videos query from the corresponding view function
(look for where related_videos is computed) to eliminate the unnecessary
database work.
- Around line 14-20: The iframe in video_detail.html embedding YouTube using {{
video_id }} lacks a title attribute which hurts screen-reader usability; update
the iframe element to include a descriptive title (e.g., use the video's title
if available like "{{ video.title }}" or a fallback such as "YouTube video") so
the iframe has a meaningful title attribute for accessibility.
- Line 8: Replace the custom wrapper classes on the page container div
(currently "mx-auto max-w-5xl px-6") with the project-standard Tailwind
container utilities; update the div in web/templates/videos/video_detail.html to
use "container mx-auto px-4" so the element uses the consistent design-system
container pattern.
- Around line 11-14: The template’s YouTube parsing is fragile and the iframe
lacks an accessible title; instead, parse and validate the YouTube video ID in
the view (reuse the regex logic from fetch_video_oembed() in views.py) and add a
context key like embed_url (e.g. "youtube_embed_url") containing the validated
"https://www.youtube.com/embed/{id}?rel=0&modestbranding=1" string; update the
view that renders video_detail to stop querying and passing related_videos if
unused, and modify the template to use the passed youtube_embed_url and include
a title attribute on the iframe (e.g. title="YouTube video player") rather than
attempting parsing in the template.
In `@web/views.py`:
- Around line 6065-6070: The new view function video_detail lacks type hints and
a docstring; update the function signature to include parameter and return type
annotations (e.g., request: HttpRequest, id: int -> HttpResponse) and add a
concise docstring at the top describing its purpose, parameters, and return
value; ensure imports used for annotations (HttpRequest, HttpResponse) are
present or use typing.TYPE_CHECKING/commented forward refs, and leave the body
logic using get_object_or_404(EducationalVideo, id=id) and render(...)
unchanged.
---
Outside diff comments:
In `@web/templates/videos/list.html`:
- Around line 105-113: The template contains a redundant `{% elif
video.thumbnail_url %}` branch that can never run because an earlier `{% if
video.thumbnail_url %}` already handles the same condition; remove the
unreachable `{% elif video.thumbnail_url %}` block (the entire duplicate
div/img/a markup) and, if any behavior from that branch needs to be preserved,
merge it into the original `{% if video.thumbnail_url %}` branch (e.g., adjust
the anchor/attributes there) — specifically inspect and consolidate the anchor
element that currently includes target="_blank" so you either remove that
attribute or ensure it's applied consistently in the remaining `if
video.thumbnail_url` markup.
- Around line 124-127: The title link currently points to external
video.video_url and opens in a new tab (href="{{ video.video_url }}" and
target="_blank"), which bypasses the internal video-detail flow; change the
anchor to point to the internal video detail route (e.g., use the app's route
helper or an internal path that references video.id or video.slug such as the
video_detail view) and remove target="_blank" so clicks stay within the app and
follow the internal video-detail flow.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
web/templates/videos/list.htmlweb/templates/videos/video_detail.htmlweb/urls.pyweb/views.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
web/templates/videos/list.html (1)
105-112:⚠️ Potential issue | 🟠 MajorRemove the unreachable duplicate thumbnail branch.
Line 105 repeats the same condition as the previous
if, so this block never executes. It also preservestarget="_blank"on Line 111, which can drift from the PR objective if this branch is later reactivated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/list.html` around lines 105 - 112, There is a duplicate unreachable branch using the same condition ({% elif video.thumbnail_url %}) that should be removed; delete the entire duplicated block that starts with "{% elif video.thumbnail_url %}" and its nested <img> and <a> elements so only the original thumbnail branch remains, and ensure no stray target="_blank" is left in any remaining thumbnail link (remove target="_blank" if present in the kept anchor).
♻️ Duplicate comments (4)
web/views.py (1)
6065-6070:⚠️ Potential issue | 🟠 MajorAdd type hints and a docstring to
video_detail.This new view is missing function annotations and a docstring, which breaks the repository’s Python standards.
♻️ Proposed fix
-def video_detail(request, id): +def video_detail(request: HttpRequest, id: int) -> HttpResponse: + """Render the educational video detail page.""" video = get_object_or_404(EducationalVideo, id=id) - # related_videos = EducationalVideo.objects.filter(category=video.category).exclude(id=id)[:5] - return render(request, "videos/video_detail.html", {"video": video})As per coding guidelines: "
**/*.py: Use type hints in Python where appropriate" and "**/*.py: Add docstrings to Python functions, classes, and modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 6065 - 6070, The view function video_detail lacks type annotations and a docstring; update its signature to include proper type hints (e.g., request: HttpRequest, id: int) and a return type (e.g., HttpResponse), add a concise docstring explaining what the view does and its parameters/return, and ensure necessary imports (HttpRequest, HttpResponse) are present; locate the function named video_detail which uses get_object_or_404(EducationalVideo, id=id) and render(...) and add the annotations and docstring there to satisfy the repository's typing and documentation standards.web/templates/videos/list.html (1)
100-103:⚠️ Potential issue | 🟡 MinorAdd an accessible name to the icon-only play link.
This anchor is icon-only; screen readers won’t announce a meaningful action without an accessible name.
Proposed fix
-<a href="{% url 'video_detail' video.id %}" - class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity"> +<a href="{% url 'video_detail' video.id %}" + aria-label="Play {{ video.title }}" + class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">As per coding guidelines, "Ensure proper HTML structure and accessibility in templates" and "Include proper ARIA labels where needed for accessibility".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/list.html` around lines 100 - 103, The icon-only play link that renders as <a href="{% url 'video_detail' video.id %}" class="absolute inset-0 flex items-center justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity"> lacks an accessible name; update this anchor to provide an accessible label by adding either an aria-label (e.g. aria-label="Play video: {{ video.title }}") or include a visually-hidden span with text like "Play video: {{ video.title }}" inside the anchor so screen readers announce the action; ensure the i.fa-play icon remains purely decorative (no title) and the accessible name includes the video title when available.web/templates/videos/video_detail.html (2)
8-8: 🛠️ Refactor suggestion | 🟠 MajorUse the project-standard container utility classes.
Proposed fix
-<div class="mx-auto max-w-5xl px-6"> +<div class="container mx-auto px-4">As per coding guidelines, "Use Tailwind container classes: container mx-auto px-4 for containers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/video_detail.html` at line 8, Replace the nonstandard container div classes on the outer wrapper (the element currently using class "mx-auto max-w-5xl px-6") with the project-standard Tailwind container utility classes by changing that div's class attribute to use "container mx-auto px-4" so it conforms to the coding guidelines.
11-24:⚠️ Potential issue | 🟠 MajorMove YouTube ID parsing out of the template and use a validated embed URL from the view.
The
cut+sliceextraction is fragile and misses valid URL formats. Parse/validate once in the view and passyoutube_embed_urlto the template.Template-side adjustment after view change
-{% if "youtube.com" in video.video_url or "youtu.be" in video.video_url %} - {% with video.video_url|cut:"https://www.youtube.com/watch?v="|cut:"https://youtu.be/" as raw_id %} - {% with raw_id|slice:":11" as video_id %} - <iframe src="https://www.youtube.com/embed/{{ video_id }}?rel=0&modestbranding=1" +{% if youtube_embed_url %} + <iframe src="{{ youtube_embed_url }}" title="YouTube video player - {{ video.title }}" frameborder="0" allow="autoplay; encrypted-media" referrerpolicy="strict-origin-when-cross-origin" allowfullscreen class="w-full h-full"> </iframe> - {% endwith %} - {% endwith %} {% endif %}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/video_detail.html` around lines 11 - 24, The template is doing fragile YouTube ID extraction using video.video_url with cut/slice; move that logic into the view (e.g., in your VideoDetailView or the function that builds the template context) where you should parse and validate video.video_url, construct a safe youtube_embed_url (e.g., "https://www.youtube.com/embed/{id}?rel=0&modestbranding=1") or set it to None if invalid, and pass youtube_embed_url in the context; then update the video_detail.html to simply check for youtube_embed_url and render the iframe src="{{ youtube_embed_url }}" without any string slicing in the template.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/videos/video_detail.html`:
- Around line 11-25: The template currently only renders an iframe for YouTube
URLs (using video.video_url, raw_id and video_id) leaving the player area empty
for non-YouTube links; add an else branch to the existing {% if "youtube.com" in
video.video_url or "youtu.be" in video.video_url %} block that renders a clear
fallback UI (e.g., a message like "External video" with a clickable link to
video.video_url and the video.title, opening in a new tab with rel="noopener
noreferrer") so users have an action path when the URL is not YouTube.
In `@web/views.py`:
- Line 6068: Remove the commented-out query line that references related_videos
and EducationalVideo from the view to reduce noise: locate the commented line
containing "related_videos =
EducationalVideo.objects.filter(category=video.category).exclude(id=id)[:5]" in
the view (where video and id are in scope) and either delete it entirely or, if
related videos are needed, reintroduce it by adding a real context entry (e.g.,
set related_videos variable and include it in the template context) otherwise
remove the commented code so no dead query remains in the request path.
---
Outside diff comments:
In `@web/templates/videos/list.html`:
- Around line 105-112: There is a duplicate unreachable branch using the same
condition ({% elif video.thumbnail_url %}) that should be removed; delete the
entire duplicated block that starts with "{% elif video.thumbnail_url %}" and
its nested <img> and <a> elements so only the original thumbnail branch remains,
and ensure no stray target="_blank" is left in any remaining thumbnail link
(remove target="_blank" if present in the kept anchor).
---
Duplicate comments:
In `@web/templates/videos/list.html`:
- Around line 100-103: The icon-only play link that renders as <a href="{% url
'video_detail' video.id %}" class="absolute inset-0 flex items-center
justify-center bg-black bg-opacity-25 hover:bg-opacity-0 transition-opacity">
lacks an accessible name; update this anchor to provide an accessible label by
adding either an aria-label (e.g. aria-label="Play video: {{ video.title }}") or
include a visually-hidden span with text like "Play video: {{ video.title }}"
inside the anchor so screen readers announce the action; ensure the i.fa-play
icon remains purely decorative (no title) and the accessible name includes the
video title when available.
In `@web/templates/videos/video_detail.html`:
- Line 8: Replace the nonstandard container div classes on the outer wrapper
(the element currently using class "mx-auto max-w-5xl px-6") with the
project-standard Tailwind container utility classes by changing that div's class
attribute to use "container mx-auto px-4" so it conforms to the coding
guidelines.
- Around line 11-24: The template is doing fragile YouTube ID extraction using
video.video_url with cut/slice; move that logic into the view (e.g., in your
VideoDetailView or the function that builds the template context) where you
should parse and validate video.video_url, construct a safe youtube_embed_url
(e.g., "https://www.youtube.com/embed/{id}?rel=0&modestbranding=1") or set it to
None if invalid, and pass youtube_embed_url in the context; then update the
video_detail.html to simply check for youtube_embed_url and render the iframe
src="{{ youtube_embed_url }}" without any string slicing in the template.
In `@web/views.py`:
- Around line 6065-6070: The view function video_detail lacks type annotations
and a docstring; update its signature to include proper type hints (e.g.,
request: HttpRequest, id: int) and a return type (e.g., HttpResponse), add a
concise docstring explaining what the view does and its parameters/return, and
ensure necessary imports (HttpRequest, HttpResponse) are present; locate the
function named video_detail which uses get_object_or_404(EducationalVideo,
id=id) and render(...) and add the annotations and docstring there to satisfy
the repository's typing and documentation standards.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
web/templates/videos/list.htmlweb/templates/videos/video_detail.htmlweb/views.py
Contributor
💬 Unresolved Review ConversationsHi @Shrey1006! 👋 This pull request currently has 1 unresolved review conversation. Please address all review feedback and push a new commit to resolve them before this PR can be merged. Steps to resolve:
Once all conversations are resolved, this notice will be removed automatically. Thank you! 🙏 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
web/views.py (1)
6065-6068: 🛠️ Refactor suggestion | 🟠 MajorAdd type hints and a docstring to
video_detail.
video_detailis missing annotations and a function docstring, which are required by project standards.♻️ Proposed fix
-def video_detail(request, id): +def video_detail(request: HttpRequest, id: int) -> HttpResponse: + """Render the video detail page for a specific educational video.""" video = get_object_or_404(EducationalVideo, id=id) return render(request, "videos/video_detail.html", {"video": video})As per coding guidelines: "
**/*.py: Use type hints in Python where appropriate" and "**/*.py: Add docstrings to Python functions, classes, and modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/views.py` around lines 6065 - 6068, The view function video_detail lacks type annotations and a docstring; add a concise one-line docstring describing that it returns the video detail page and annotate parameters and return type: type-hint request as HttpRequest, id as int (or str if URLs use slug), and the return as HttpResponse; also import typing names if needed and mention EducationalVideo, get_object_or_404, and render remain unchanged so the function signature becomes def video_detail(request: HttpRequest, id: int) -> HttpResponse and include a short docstring explaining behavior and what is returned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/videos/video_detail.html`:
- Around line 51-52: The template currently accesses video.category.name which
will raise if video.category is None; update the video_detail.html to guard this
access by checking if video.category exists (e.g., conditional with {% if
video.category %}...{% else %}...) or use the Django default filter on the value
(e.g., render video.category.name|default:"Uncategorized") so the template never
attempts to access .name on None.
- Around line 29-33: Replace the fallback button's non-compliant Tailwind
classes and missing security attribute: in the anchor for the video link (the
<a> element that currently uses bg-orange-500 hover:bg-orange-600 text-white
py-2 px-4 rounded-lg transition-colors), swap its classes to the project's
primary button classes (bg-teal-300 hover:bg-teal-400 text-white px-6 py-2
rounded-lg transition duration-200) and add rel="noopener noreferrer" alongside
target="_blank" to the same <a> element to satisfy the color scheme and security
guideline.
---
Duplicate comments:
In `@web/views.py`:
- Around line 6065-6068: The view function video_detail lacks type annotations
and a docstring; add a concise one-line docstring describing that it returns the
video detail page and annotate parameters and return type: type-hint request as
HttpRequest, id as int (or str if URLs use slug), and the return as
HttpResponse; also import typing names if needed and mention EducationalVideo,
get_object_or_404, and render remain unchanged so the function signature becomes
def video_detail(request: HttpRequest, id: int) -> HttpResponse and include a
short docstring explaining behavior and what is returned.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
web/templates/videos/video_detail.html (2)
8-8:⚠️ Potential issue | 🟠 MajorUse the project-standard container utility classes.
The page wrapper still uses a custom container pattern and should be normalized to the required design-system container utilities.
Proposed fix
- <div class="mx-auto max-w-5xl px-6"> + <div class="container mx-auto px-4">As per coding guidelines, "Use Tailwind container classes: container mx-auto px-4 for containers".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/video_detail.html` at line 8, The outer wrapper div in the video_detail template uses a custom class string ("mx-auto max-w-5xl px-6"); replace that with the project-standard container utility classes by changing the wrapper's class attribute to "container mx-auto px-4" (i.e., update the div that currently has class="mx-auto max-w-5xl px-6" to class="container mx-auto px-4") so the template uses the design-system container utilities.
11-23:⚠️ Potential issue | 🟠 MajorStop parsing YouTube IDs in the template; use a validated embed URL from the view.
cut+sliceis brittle and will break for valid YouTube URL variants. Pass a precomputedyoutube_embed_urlfrom the view and render that directly.Proposed template-side change
- {% if "youtube.com" in video.video_url or "youtu.be" in video.video_url %} - {% with video.video_url|cut:"https://www.youtube.com/watch?v="|cut:"https://youtu.be/" as raw_id %} - {% with raw_id|slice:":11" as video_id %} - <iframe src="https://www.youtube.com/embed/{{ video_id }}?rel=0&modestbranding=1" - frameborder="0" - title="YouTube video player - {{ video.title }}" - allow="autoplay; encrypted-media" - referrerpolicy="strict-origin-when-cross-origin" - allowfullscreen - class="w-full h-full"> - </iframe> - {% endwith %} - {% endwith %} + {% if youtube_embed_url %} + <iframe src="{{ youtube_embed_url }}" + frameborder="0" + title="YouTube video player - {{ video.title }}" + allow="autoplay; encrypted-media" + referrerpolicy="strict-origin-when-cross-origin" + allowfullscreen + class="w-full h-full"> + </iframe>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/templates/videos/video_detail.html` around lines 11 - 23, The template currently parses YouTube IDs using brittle filters (video.video_url with cut and slice producing video_id) and should instead render a validated embed URL supplied by the view; stop using the nested {% with %} blocks and the video_id variable and replace the iframe src to use a view-provided youtube_embed_url (or similar variable named youtube_embed_url) that the view computes/validates, ensuring the iframe title still uses video.title and other attributes remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@web/templates/videos/video_detail.html`:
- Line 44: Replace the off-palette Tailwind color tokens on the metadata div:
locate the element that currently uses the classes "text-sm text-gray-500
dark:text-gray-400 mb-4" and change only the color classes to the project's body
text classes "text-gray-600 dark:text-gray-300" (keeping text size and margin
classes intact) so the div becomes "text-sm text-gray-600 dark:text-gray-300
mb-4".
---
Duplicate comments:
In `@web/templates/videos/video_detail.html`:
- Line 8: The outer wrapper div in the video_detail template uses a custom class
string ("mx-auto max-w-5xl px-6"); replace that with the project-standard
container utility classes by changing the wrapper's class attribute to
"container mx-auto px-4" (i.e., update the div that currently has class="mx-auto
max-w-5xl px-6" to class="container mx-auto px-4") so the template uses the
design-system container utilities.
- Around line 11-23: The template currently parses YouTube IDs using brittle
filters (video.video_url with cut and slice producing video_id) and should
instead render a validated embed URL supplied by the view; stop using the nested
{% with %} blocks and the video_id variable and replace the iframe src to use a
view-provided youtube_embed_url (or similar variable named youtube_embed_url)
that the view computes/validates, ensuring the iframe title still uses
video.title and other attributes remain unchanged.
Contributor
Author
|
Hi @A1L13N I noticed that the “Peer Review Reminder / Check PR has a peer review (pull_request_review)” is currently failing on this PR. Could you clarify why it’s failing? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
on clicking on the video card it opened Youtube on other tab
i made a inbuild Video player so that people dont need to go on youtube
i have attached video of new page made and also old method of playing video
Related issues
Fixes #<ISSUE_NUMBER>
Checklist
-> added New Video_detail page
-> linked the new page to the Video_detail page
-> now no need to go Youtube and get distracted
after:
https://github.com/user-attachments/assets/32f21a67-768b-4f7c-bd25-6055529d7d4b
before :
Summary by CodeRabbit