Update VideoDemo component dimensions and improve layout responsiveness#283
Update VideoDemo component dimensions and improve layout responsiveness#283
Conversation
- Adjusted video dimensions to 2048x1672 for better quality. - Updated aspect ratio calculation for the video button to enhance responsiveness. - Increased max width of the surrounding container to 6xl for improved layout on larger screens. These changes aim to provide a better user experience by ensuring the video demo displays correctly across various screen sizes.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughIncrease demo video source dimensions and container width; replace hard-coded video size with an aspect-ratio helper; adjust fallback image sizing and tests; add small Playwright timing/timeout adjustments for flakiness. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Page as Home Page
participant Comp as VideoDemo
participant Helper as getAspectRatio()
participant DOM as Browser DOM
Page->>Comp: render VideoDemo
Comp->>Helper: getAspectRatio(2048,1672)
Helper-->>Comp: "2048 / 1672"
Comp->>DOM: set container style aspect-ratio:"2048 / 1672" and class w-full mx-auto
alt video available
Comp->>DOM: render video element (2048x1672 source)
else video unavailable
Comp->>DOM: render fallback Image with sizes="100vw"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/www/components/page/video-demo.tsx (2)
12-13: Consider inlining the helper or adding a GCD-based simplification.The
getAspectRatiohelper is used only once. You could either:
- Inline it directly:
style={{ aspectRatio: \${WIDTH} / ${HEIGHT}` }}`- Or enhance it to simplify ratios (2048/1672 reduces to 128/104.5, though the current form is acceptable for CSS).
If you prefer inlining:
-const getAspectRatio = (width: number, height: number) => - `${width} / ${height}`;And on line 36:
- style={{ aspectRatio: getAspectRatio(WIDTH, HEIGHT) }} + style={{ aspectRatio: `${WIDTH} / ${HEIGHT}` }}
44-44: Restore responsivesizesfor better performance.Changing from
(max-width: 768px) 100vw, 800pxto100vwtells the browser to always download a full-viewport-width image, even on large screens where the container is constrained tomax-w-6xl(1152px). This can waste bandwidth and increase load times.Apply this diff to restore responsive sizing with the new constraint:
- sizes="100vw" + sizes="(max-width: 1152px) 100vw, 1152px"Alternatively, use Tailwind's
max-w-6xlbreakpoint equivalent (1280px):- sizes="100vw" + sizes="(max-width: 1280px) 100vw, 1152px"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/www/app/(home)/page.tsx(1 hunks)apps/www/components/page/video-demo.tsx(2 hunks)
⏰ 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). (2)
- GitHub Check: test-e2e (e2e)
- GitHub Check: test-e2e (a11y)
🔇 Additional comments (4)
apps/www/components/page/video-demo.tsx (3)
29-29: LGTM: Container layout now more flexible.Removing the hard-coded
max-w-[800px]and usingw-fullwithmx-autocorrectly delegates width control to the parent container, which now usesmax-w-6xl.
36-36: LGTM: Dynamic aspect ratio maintains correct proportions.Using the
getAspectRatiohelper in the inline style ensures the button container maintains the correct aspect ratio regardless of screen size.
8-10: Verify video and GIF dimensions match the 2048×1672 constants.The constants are labeled "Actual video dimensions," but we cannot definitively verify them in this environment. The video is hosted on Mux (external streaming service, not local), and the metadata file lacks dimension information. While the GIF file exists at
/assets/demo.gif, the sandbox lacks tools to inspect its dimensions directly. Please manually confirm that both the video source and the GIF fallback have been updated to exactly 2048×1672 pixels to avoid visual mismatches or distortion.apps/www/app/(home)/page.tsx (1)
43-43: LGTM: Container width increase aligns with PR objectives.Changing from
max-w-5xl(1024px) tomax-w-6xl(1152px) provides a ~12.5% increase in maximum width, which falls within the requested 10-20% size increase for the video demo.
- Removed the max width constraint from the container for improved layout flexibility. - Updated the button's aspect ratio to reflect the new video dimensions of 2048x1672. - Adjusted the fallback image's sizes attribute for better responsiveness. These changes enhance the test coverage for the VideoDemo component, ensuring it accurately reflects the latest design updates.
…curacy - Added a wait timeout in the homepage test to reduce flakiness when loading the video. - Updated the responsive test to reflect the new maximum width constraint for videos, adjusting it to 1300px for better layout consistency. These changes aim to enhance the stability of the tests and ensure they accurately represent the latest design specifications.
github-actions
bot
added
the
tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tooling/playwright-www/tests/homepage.test.ts (1)
146-147: Replace arbitrary timeout with condition-based waiting.Hardcoded timeouts like
waitForTimeout(500)are brittle and can still be flaky. Instead, wait for a specific condition that indicates the video is ready for interaction.Consider replacing the arbitrary delay with a condition-based wait:
- // Wait a bit for video to be fully loaded to reduce flakiness - await page.waitForTimeout(500); + // Wait for video to be ready for interaction + await videoButton.locator('video').evaluate((video: HTMLVideoElement) => { + return video.readyState >= 2; // HAVE_CURRENT_DATA or higher + }).catch(() => { + // Fallback if video doesn't exist (e.g., using GIF fallback) + });Alternatively, if the flakiness is related to event handlers, ensure the component's JavaScript has hydrated:
- // Wait a bit for video to be fully loaded to reduce flakiness - await page.waitForTimeout(500); + // Wait for interactive hydration + await page.waitForLoadState('domcontentloaded');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/www/components/page/video-demo.test.tsx(2 hunks)tooling/playwright-www/tests/homepage.test.ts(1 hunks)tooling/playwright-www/tests/responsive.test.ts(1 hunks)
⏰ 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). (2)
- GitHub Check: test-e2e (e2e)
- GitHub Check: test-e2e (a11y)
🔇 Additional comments (4)
apps/www/components/page/video-demo.test.tsx (2)
347-347: LGTM! Aspect ratio correctly updated.The test expectation correctly reflects the new video dimensions (2048×1672) mentioned in the PR objectives.
363-363: Verify the performance impact of removing responsive image sizing.The sizes attribute changed from
"(max-width: 768px) 100vw, 800px"to"100vw". This removes the optimization that capped image width at 800px on larger screens, potentially causing the browser to load unnecessarily large images on desktop devices where the image is displayed at a smaller size.Consider whether the fallback GIF should retain responsive sizing like
"(max-width: 768px) 100vw, (max-width: 1536px) 1280px, 100vw"to align with the max-w-6xl container while avoiding over-fetching on smaller viewports.tooling/playwright-www/tests/homepage.test.ts (1)
151-151: LGTM! Appropriate timeout for external navigation.The 10-second timeout is reasonable for waiting on a new page to open when navigating to StackBlitz, as external sites can have variable load times.
tooling/playwright-www/tests/responsive.test.ts (1)
172-174: LGTM! Correctly reflects the max-w-6xl container change.The updated assertion aligns with the container width increase from max-w-5xl to max-w-6xl (1280px in Tailwind), with appropriate tolerance for padding and sub-pixel rendering.
1 participant
These changes aim to provide a better user experience by ensuring the video demo displays correctly across various screen sizes.
Closes #282
Summary by CodeRabbit
Style
Tests