Add video section #9
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new video section component to the landing page builder, supporting both YouTube embeds and native HTML5 video playback. It also refactors existing section components to use a new shared Section wrapper component, eliminating code duplication and improving maintainability.
- Introduces a new
Videosection component with YouTube URL parsing and native video support - Creates a reusable
Sectioncomponent to standardize section layouts across the codebase - Adds
AspectRatiocomponent wrapper for responsive video sizing
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ui/sections/video.tsx | New video section with YouTube embed detection and HTML5 video fallback |
| src/ui/components/section.tsx | New shared section wrapper component consolidating title/subtitle rendering |
| src/ui/components/aspect-ratio.tsx | Wrapper for Radix UI aspect ratio primitive |
| src/configuration/types/sections/video-section.ts | Type definitions for video section configuration |
| src/ui/sections/terminal.tsx | Refactored to use shared Section component |
| src/ui/sections/steps.tsx | Refactored to use shared Section component |
| src/ui/sections/hero.tsx | Refactored to use shared Section component |
| src/ui/sections/faq.tsx | Refactored to use shared Section component |
| src/ui/sections/cards.tsx | Refactored to use shared Section component |
| src/ui/components/section-title.tsx | Removed (functionality moved to Section component) |
| src/sections.tsx | Added video section to the sections registry |
| src/stories/sections/Video.stories.tsx | Storybook stories for video section variants |
| package.json | Added @radix-ui/react-aspect-ratio dependency |
| package-lock.json | Dependency lockfile updates |
| configuration.yaml | Example video section configuration |
| README.md | Documentation updates for video section and typo fixes |
| .storybook/preview.tsx | Changed width from w-screen to w-full for better preview layout |
| .github/PULL_REQUEST_TEMPLATE.md | Removed heading from PR template |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| } catch { | ||
| // unexpected absolute URL — fallthrough to string checks below |
There was a problem hiding this comment.
The comment says "unexpected absolute URL" but the code only handles URL parsing errors, not specifically absolute URLs. The comment should be more accurate, such as "Invalid URL format - fallthrough to string checks below" or "URL parsing failed - fallthrough to string pattern matching".
| // unexpected absolute URL — fallthrough to string checks below | |
| // URL parsing failed — fallthrough to string pattern matching below |
| if (src.includes('/embed/')) return src; | ||
| const match = src.match(/[?&]v=([^&]+)/); | ||
| if (match?.[1]) return `https://www.youtube.com/embed/${match[1]}`; | ||
| const short = src.match(/youtu\.be\/(.+)$/); |
There was a problem hiding this comment.
The regex pattern /youtu\.be\/(.+)$/ is too greedy and will capture query parameters and fragments as part of the video ID. For example, youtu.be/abc123?t=10 would capture abc123?t=10 as the ID, which is incorrect. Consider using /youtu\.be\/([^?&#]+) to stop at query parameters or fragments.
| const short = src.match(/youtu\.be\/(.+)$/); | |
| const short = src.match(/youtu\.be\/([^?&#]+)/); |
| subtitle: See how easy it is to create a landing page with Qupid. | ||
| video: | ||
| src: https://www.youtube.com/embed/xz5jZ1zwFC4 | ||
| ratio: 1.7777777777778 # 16 / 9 aspect ratio |
There was a problem hiding this comment.
[nitpick] The hardcoded ratio value 1.7777777777778 could introduce floating-point precision inconsistencies. Since the default ratio in the code is 16 / 9, it would be clearer and more maintainable to use a comment-only explanation and let the default apply, or use a simpler value like 1.778. Alternatively, consider supporting string ratios like "16:9" in the configuration for better readability.
| ratio: 1.7777777777778 # 16 / 9 aspect ratio | |
| ratio: "16:9" # aspect ratio |
| @@ -0,0 +1,9 @@ | |||
| import * as AspectRatioPrimitive from "@radix-ui/react-aspect-ratio" | |||
There was a problem hiding this comment.
[nitpick] Inconsistent quote style. The file uses double quotes for the import statement, but the rest of the codebase appears to use single quotes. Consider using single quotes for consistency: import * as AspectRatioPrimitive from '@radix-ui/react-aspect-ratio'
| import * as AspectRatioPrimitive from "@radix-ui/react-aspect-ratio" | |
| import * as AspectRatioPrimitive from '@radix-ui/react-aspect-ratio' |
| <video | ||
| className="w-full h-auto rounded-lg mx-auto" | ||
| controls | ||
| playsInline |
There was a problem hiding this comment.
The video element lacks a title or aria-label attribute for accessibility. While the iframe has a descriptive title, the native video element should also have an accessible name to help screen reader users understand what video they're interacting with. Consider adding: aria-label={iframeTitle} or similar.
| playsInline | |
| playsInline | |
| aria-label={iframeTitle} |
| {(title || subtitle) && ( | ||
| <div className="mb-8 text-center"> | ||
| {title && ( | ||
| <Markdown | ||
| content={`## ${title}`} | ||
| className="text-3xl font-bold mb-4" | ||
| /> | ||
| )} | ||
| {subtitle && ( | ||
| <Markdown | ||
| content={subtitle} | ||
| className="text-lg text-muted-foreground" | ||
| /> | ||
| )} | ||
| </div> |
There was a problem hiding this comment.
[nitpick] The condition (title || subtitle) will render an empty div with margins when only one of title or subtitle exists but is an empty string. While this is a minor edge case, consider using truthy checks that also exclude empty strings: (title?.trim() || subtitle?.trim()) or similar, to avoid unnecessary empty divs with margins.
Type of Change
Description
Add video section.