Add themed brand lockups and simplify git-rain winget install#5
Merged
Merged
Conversation
Serve git-fire and git-rain lockup SVGs from public/assets with light/dark variants tied to data-theme and prefers-color-scheme. Add BrandLockup across layouts and the header. Prefer winget install git-rain with a fallback note. Ignore Windows Zone.Identifier streams. Made-with: Cursor
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 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 docstrings
🧪 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 |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
git-fire-website | 2841964 | Commit Preview URL Branch Preview URL |
Apr 17 2026, 12:57 AM |
![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 (2)
src/styles/global.css (1)
130-140: Deprecatedclipproperty (stylelint).Stylelint flags
clip: rect(0, 0, 0, 0)as deprecated. The modern equivalent isclip-path: inset(50%). Keeping both covers older engines, but the common minimal pattern today is justclip-path.Suggested fix
.visually-hidden { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap; border-width: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/global.css` around lines 130 - 140, Update the .visually-hidden rule to remove the deprecated clip usage and use the modern clip-path instead: replace the deprecated "clip: rect(0, 0, 0, 0)" with "clip-path: inset(50%)" (you may optionally keep the old clip declaration before clip-path for older engines, but ensure clip-path is present and takes precedence); keep the rest of the properties in the .visually-hidden selector unchanged so accessibility behavior remains intact.src/components/BrandLockup.astro (1)
21-36: Both SVGs are always fetched; consider using<picture>withmediaqueries.The light and dark lockups are rendered as two
<img>tags with CSS togglingdisplaybased on theme. Browsers still fetch the hidden image, so every page load downloads both artworks for each lockup (header + up to two section lockups = up to 6 SVGs). A<picture>with<source media="(prefers-color-scheme: dark)">(plus a[data-theme]story) would let the browser pick one, but the CSS-swap approach is also needed to honor the in-page theme toggle, so this is a nice-to-have rather than a must-fix. At minimum, SVGs are small enough that this is a minor perf concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/BrandLockup.astro` around lines 21 - 36, BrandLockup currently renders two <img> tags (classes brand-lockup__img--light and --dark) which causes both SVGs to be fetched; change rendering to use a <picture> element with a <source media="(prefers-color-scheme: dark)" srcset={`/assets/${slug}-lockup-dark.svg`} and a fallback <img src={`/assets/${slug}-lockup.svg`} alt={label} ...> so the browser selects one asset by default, and keep the existing CSS/theme classes or data-theme toggle logic to handle in-page theme switching; ensure width/height/decoding attributes from the original <img> are preserved on the fallback <img> and that slug and label are used 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 `@src/components/BrandLockup.astro`:
- Line 14: In the BrandLockup component the computed label (const label) is
applied to both SVG/img variants when decorative is false, causing duplicate
announcements; update the rendering so only the visually primary image receives
the non-empty alt (use the existing label), while the hidden variant always has
alt="" and aria-hidden="true" (keep decorative behavior unchanged when
decorative is true), adjusting the logic around the label variable and the two
<img>/<svg> elements referenced in BrandLockup to apply the empty-alt +
aria-hidden to the hidden variant (also apply the same change where those images
are rendered on the other occurrences noted).
---
Nitpick comments:
In `@src/components/BrandLockup.astro`:
- Around line 21-36: BrandLockup currently renders two <img> tags (classes
brand-lockup__img--light and --dark) which causes both SVGs to be fetched;
change rendering to use a <picture> element with a <source
media="(prefers-color-scheme: dark)" srcset={`/assets/${slug}-lockup-dark.svg`}
and a fallback <img src={`/assets/${slug}-lockup.svg`} alt={label} ...> so the
browser selects one asset by default, and keep the existing CSS/theme classes or
data-theme toggle logic to handle in-page theme switching; ensure
width/height/decoding attributes from the original <img> are preserved on the
fallback <img> and that slug and label are used unchanged.
In `@src/styles/global.css`:
- Around line 130-140: Update the .visually-hidden rule to remove the deprecated
clip usage and use the modern clip-path instead: replace the deprecated "clip:
rect(0, 0, 0, 0)" with "clip-path: inset(50%)" (you may optionally keep the old
clip declaration before clip-path for older engines, but ensure clip-path is
present and takes precedence); keep the rest of the properties in the
.visually-hidden selector unchanged so accessibility behavior remains intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9279dd29-516d-4515-a64c-b4b96fd95cb5
⛔ Files ignored due to path filters (8)
public/assets/git-fire-icon-dark.svgis excluded by!**/*.svgpublic/assets/git-fire-icon.svgis excluded by!**/*.svgpublic/assets/git-fire-lockup-dark.svgis excluded by!**/*.svgpublic/assets/git-fire-lockup.svgis excluded by!**/*.svgpublic/assets/git-rain-icon-dark.svgis excluded by!**/*.svgpublic/assets/git-rain-icon.svgis excluded by!**/*.svgpublic/assets/git-rain-lockup-dark.svgis excluded by!**/*.svgpublic/assets/git-rain-lockup.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
.gitignoresrc/components/BrandLockup.astrosrc/components/LayoutA.astrosrc/components/LayoutB.astrosrc/components/LayoutC.astrosrc/components/LayoutCurrent.astrosrc/layouts/BaseLayout.astrosrc/scripts/install-pickers.tssrc/styles/global.css
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Zero-length commit stem line renders nothing in rain lockups
- The stem line now runs from the merge point (y=61.5) to the commit center (y=68) and is drawn after the node circle so it remains visible.
You can send follow-ups to the cloud agent here.
Reviewed by Cursor Bugbot for commit 38456d5. Configure here.
Extend the merge-point stem to the commit node center and paint the stem after the node circle so the segment is not fully covered.
Scale up sm/md/lg wordmarks, add soft brand-tinted plates in hero cards, accent underlines on section and feature-row headings, and header glow matched to featured product. Made-with: Cursor
Scale header and feature/rich copy wordmarks; add fire/rain top bands for section--rich (layout C). Rain-featured pages use a rain-only hero bleed. US spelling: behaviour -> behavior in shared layout copy.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/styles/global.css (2)
140-150: Replace deprecatedclipwithclip-pathin.visually-hidden.Stylelint flags
clipas deprecated (property-no-deprecated). Modern visually-hidden patterns useclip-path: inset(50%)instead.♻️ Proposed fix
.visually-hidden { position: absolute; width: 1px; height: 1px; padding: 0; margin: -1px; overflow: hidden; - clip: rect(0, 0, 0, 0); + clip-path: inset(50%); white-space: nowrap; border-width: 0; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/global.css` around lines 140 - 150, The .visually-hidden rule uses the deprecated clip property; update the declaration in the .visually-hidden selector to remove clip: rect(0, 0, 0, 0) and use a modern equivalent such as clip-path: inset(50%) (and keep the existing position/size/overflow rules intact); ensure the selector .visually-hidden is edited so stylelint no longer flags property-no-deprecated while preserving the intended visually-hidden behavior.
184-219: Both light and dark lockup SVGs are always downloaded.Toggling visibility via
display: noneon<img>elements still triggers a network fetch for both*-lockup.svgand*-lockup-dark.svgon every page (header lockup + each layout’s lockups → ~6+ extra requests). Consider one of:
- Render only the active variant via a small inline-script/SSR check, or
- Use a single SVG that adapts via CSS custom properties /
currentColorandprefers-color-scheme, or- Use
<picture>withmedia="(prefers-color-scheme: dark)"so the browser only fetches the matching source (note: this won’t respect the manualdata-themetoggle without JS swappingsrcset).Not a blocker for static SVGs that gzip well, but worth flagging since the assets are duplicated everywhere
BrandLockupappears.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/styles/global.css` around lines 184 - 219, Both SVG variants are always fetched because using display:none on <img> doesn’t prevent network requests; update the BrandLockup rendering to only deliver one source to the browser. Replace the paired <img class="brand-lockup__img--light"> and <img class="brand-lockup__img--dark"> with a <picture> containing <source media="(prefers-color-scheme: dark)" srcset="...-dark.svg"> and a default <img class="brand-lockup__img" src="...-light.svg"> so the browser only downloads the matching file, and additionally wire the component (BrandLockup) to respect manual theme toggles by updating the picture's srcset via the existing theme toggle handler (or SSR/render server-side the correct src when data-theme is known) so data-theme changes set the correct srcset instead of toggling display.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/styles/global.css`:
- Around line 140-150: The .visually-hidden rule uses the deprecated clip
property; update the declaration in the .visually-hidden selector to remove
clip: rect(0, 0, 0, 0) and use a modern equivalent such as clip-path: inset(50%)
(and keep the existing position/size/overflow rules intact); ensure the selector
.visually-hidden is edited so stylelint no longer flags property-no-deprecated
while preserving the intended visually-hidden behavior.
- Around line 184-219: Both SVG variants are always fetched because using
display:none on <img> doesn’t prevent network requests; update the BrandLockup
rendering to only deliver one source to the browser. Replace the paired <img
class="brand-lockup__img--light"> and <img class="brand-lockup__img--dark"> with
a <picture> containing <source media="(prefers-color-scheme: dark)"
srcset="...-dark.svg"> and a default <img class="brand-lockup__img"
src="...-light.svg"> so the browser only downloads the matching file, and
additionally wire the component (BrandLockup) to respect manual theme toggles by
updating the picture's srcset via the existing theme toggle handler (or
SSR/render server-side the correct src when data-theme is known) so data-theme
changes set the correct srcset instead of toggling display.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dc07a5b9-83d6-41c8-8a75-d35bda83b792
⛔ Files ignored due to path filters (2)
public/assets/git-rain-lockup-dark.svgis excluded by!**/*.svgpublic/assets/git-rain-lockup.svgis excluded by!**/*.svg
📒 Files selected for processing (5)
src/components/LayoutA.astrosrc/components/LayoutB.astrosrc/components/LayoutC.astrosrc/components/LayoutCurrent.astrosrc/styles/global.css
🚧 Files skipped from review as they are similar to previous changes (3)
- src/components/LayoutB.astro
- src/components/LayoutC.astro
- src/components/LayoutCurrent.astro
BrandLockup: single aria-label on wrapper when not decorative; empty img alts. Rain lockups: draw stem segment above commit node (Bugbot). Replace deprecated clip with clip-path for .visually-hidden (CodeRabbit).
Member
Author
|
Follow-up from latest CodeRabbit review (
|
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.
Summary
public/assets/, with light and dark artwork selected viadata-themeandprefers-color-scheme(same behavior as the rest of the site).BrandLockupcomponent and use it in the header (featured product) and in every layout next to the fire/rain headings (hero cards, feature rows, rich sections, and dedicated sections).winget install git-rain, with the fully-qualified package id noted as a fallback (aligned with the git-fire winget pattern)..gitignore: ignore*Zone.Identifierso Windows download metadata is not committed.Validation
npm run buildpasses locally.Notes
assets/copies remain untracked; canonical paths for the site are underpublic/assets/.Note
Low Risk
Mostly static asset/CSS/markup changes for branding plus a minor install-command string update; low risk aside from potential visual regressions or broken asset paths.
Overview
Adds theme-aware
git-fire/git-rainbrand SVGs (icons + lockups) underpublic/assets/and introduces a reusableBrandLockupAstro component that swaps light/dark artwork via theme.Updates all layouts and
BaseLayout’s header to use the new lockups, and expandsglobal.csswith sizing, theme switching, and fire/rain-specific styling (including a rain-only hero glow when rain is featured). Also tweaks Windows install copy to preferwinget install git-rainwith a fallback ID, fixes “behaviour” → “behavior”, and extends.gitignoreto drop*Zone.Identifierand repo-root/assets/.Reviewed by Cursor Bugbot for commit 2841964. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Style
Chores