new home page#117
Draft
str4wb3rrytea wants to merge 2 commits into
Draft
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Merged
WalkthroughA new VideoBackground React component is introduced with accompanying CSS module styling. The component accepts an image URL and children content, rendering them within a structured layout with positioned overlays and interactive button/link states. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
🧹 Nitpick comments (1)
src/components/VideoBackground/VideoBackground.tsx (1)
5-25: Solid, simple API; consider aligning names/comment with actual behaviorThe component implementation and typing look good:
image, optionalstyle, andchildrenare clear, and the layout matches the CSS module usage.Minor clarity nits:
- The props interface is named
BannerPhotoPropswhile the component isVideoBackground; either renaming the interface toVideoBackgroundPropsor updating the component name would reduce cognitive overhead.- The comment says “background video” but the component uses a background image (
backgroundImage: url(...)). Updating the comment (or future-proofing the implementation if you do intend to support actual<video>) would avoid confusion for the next reader.These are non-blocking, but worth tightening up while this file is new.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
public/Rocketry Launch Website BG.mp4is excluded by!**/*.mp4
📒 Files selected for processing (2)
src/components/VideoBackground/VideoBackground.module.css(1 hunks)src/components/VideoBackground/VideoBackground.tsx(1 hunks)
Comment on lines
+35
to
+83
| .bannerText button{ | ||
| position: absolute; | ||
| top: 62%; | ||
| left: 10%; | ||
| transform: translateX(-50%); | ||
| color: white; | ||
| display: flex; | ||
| justify-content: center; | ||
| font-size: 20px; | ||
| gap: 10px; | ||
| background-color: #dbb548; | ||
| width: 175px; | ||
| height: 50px; | ||
| padding: 15px 30px; | ||
| border-radius: 10px; | ||
| border: none; | ||
| align-items: center; | ||
| transition: background-color 0.4s ease; | ||
| } | ||
|
|
||
| .bannerText button:hover, | ||
| .bannerText button:active{ | ||
| background-color: #b8963b; | ||
| outline: none; | ||
| } | ||
|
|
||
| .bannerText a{ | ||
| position: absolute; | ||
| top: 62%; | ||
| left: 30%; | ||
| transform: translateX(-50%); | ||
| display: flex; | ||
| justify-content: center; | ||
| font-size: 20px; | ||
| gap: 10px; | ||
| background-color: #dbb548; | ||
| width: 175px; | ||
| height: 50px; | ||
| padding: 15px 30px; | ||
| border-radius: 10px; | ||
| border: none; | ||
| align-items: center; | ||
| transition: background-color 0.4s ease; | ||
| } | ||
|
|
||
| .bannerText a:hover, | ||
| .bannerText a:active { | ||
| background-color: #b8963b; | ||
| } |
There was a problem hiding this comment.
Add visible focus styles for keyboard accessibility (and optionally dedupe CTA styles)
The visual styling for the heading and CTAs looks coherent, but two accessibility issues are worth fixing:
.bannerText button:hover, .bannerText button:activesetsoutline: noneand there is no:focus/:focus-visiblerule, so keyboard users may get no visible focus indication..bannerText ais styled like a button but also lacks explicit focus styling.
You can keep the current hover/active behavior and add focus-visible styles like:
-.bannerText button:hover,
-.bannerText button:active{
- background-color: #b8963b;
- outline: none;
-}
+.bannerText button:hover,
+.bannerText button:active,
+.bannerText button:focus-visible {
+ background-color: #b8963b;
+}
+
+.bannerText button:focus-visible {
+ outline: 2px solid #fff;
+ outline-offset: 2px;
+}
+
+.bannerText a:hover,
+.bannerText a:active {
+ background-color: #b8963b;
+}
+
+.bannerText a:focus-visible {
+ outline: 2px solid #fff;
+ outline-offset: 2px;
+}(Adjust colors/thickness to match your design system.)
Optional: since .bannerText button and .bannerText a share nearly identical box styles, consider extracting a shared class (e.g., .bannerCta) to reduce duplication if you’re willing to touch the JSX.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| .bannerText button{ | |
| position: absolute; | |
| top: 62%; | |
| left: 10%; | |
| transform: translateX(-50%); | |
| color: white; | |
| display: flex; | |
| justify-content: center; | |
| font-size: 20px; | |
| gap: 10px; | |
| background-color: #dbb548; | |
| width: 175px; | |
| height: 50px; | |
| padding: 15px 30px; | |
| border-radius: 10px; | |
| border: none; | |
| align-items: center; | |
| transition: background-color 0.4s ease; | |
| } | |
| .bannerText button:hover, | |
| .bannerText button:active{ | |
| background-color: #b8963b; | |
| outline: none; | |
| } | |
| .bannerText a{ | |
| position: absolute; | |
| top: 62%; | |
| left: 30%; | |
| transform: translateX(-50%); | |
| display: flex; | |
| justify-content: center; | |
| font-size: 20px; | |
| gap: 10px; | |
| background-color: #dbb548; | |
| width: 175px; | |
| height: 50px; | |
| padding: 15px 30px; | |
| border-radius: 10px; | |
| border: none; | |
| align-items: center; | |
| transition: background-color 0.4s ease; | |
| } | |
| .bannerText a:hover, | |
| .bannerText a:active { | |
| background-color: #b8963b; | |
| } | |
| .bannerText button{ | |
| position: absolute; | |
| top: 62%; | |
| left: 10%; | |
| transform: translateX(-50%); | |
| color: white; | |
| display: flex; | |
| justify-content: center; | |
| font-size: 20px; | |
| gap: 10px; | |
| background-color: #dbb548; | |
| width: 175px; | |
| height: 50px; | |
| padding: 15px 30px; | |
| border-radius: 10px; | |
| border: none; | |
| align-items: center; | |
| transition: background-color 0.4s ease; | |
| } | |
| .bannerText button:hover, | |
| .bannerText button:active, | |
| .bannerText button:focus-visible { | |
| background-color: #b8963b; | |
| } | |
| .bannerText button:focus-visible { | |
| outline: 2px solid #fff; | |
| outline-offset: 2px; | |
| } | |
| .bannerText a{ | |
| position: absolute; | |
| top: 62%; | |
| left: 30%; | |
| transform: translateX(-50%); | |
| display: flex; | |
| justify-content: center; | |
| font-size: 20px; | |
| gap: 10px; | |
| background-color: #dbb548; | |
| width: 175px; | |
| height: 50px; | |
| padding: 15px 30px; | |
| border-radius: 10px; | |
| border: none; | |
| align-items: center; | |
| transition: background-color 0.4s ease; | |
| } | |
| .bannerText a:hover, | |
| .bannerText a:active { | |
| background-color: #b8963b; | |
| } | |
| .bannerText a:focus-visible { | |
| outline: 2px solid #fff; | |
| outline-offset: 2px; | |
| } |
🤖 Prompt for AI Agents
In src/components/VideoBackground/VideoBackground.module.css around lines 35 to
83, the CTA button and anchor remove the default focus outline and lack any
focus/:focus-visible styles, leaving keyboard users without a visible focus
indicator; add explicit :focus and :focus-visible rules for both .bannerText
button and .bannerText a (or better, create a shared .bannerCta class) that
restore a clear visible focus ring (for example a 2px solid or an accessible
box-shadow ring with sufficient contrast), remove the blanket outline: none on
focus states, and keep existing hover/active colors; optionally refactor the
duplicated styles into a shared class applied to both the button and anchor to
reduce duplication if you update the JSX.
str4wb3rrytea
force-pushed
the
landing-page
branch
from
3cbb859 to
f90d0ee
Compare
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
2 participants
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.
Description
I changed X and Y to accomplish Z.
This PR closes #00.
Developer Testing
Testing done:
Reviewer Testing
Steps to view/test Feature X:
git clonethis reponpm installnpm startSteps to view/test Feature Y:
This change is
Summary by CodeRabbit
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.