Merged
Conversation
Deploying spicetify-docs with
|
| Latest commit: |
da3ba89
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://36f5e96d.spicetify-docs.pages.dev |
| Branch Preview URL: | https://feat-hero-restyling.spicetify-docs.pages.dev |
WalkthroughAdds two SVG icon components, exposes a discordMembers prop and renders an inline member-count badge in the footer, and implements a substantial homepage redesign (new hero, showcase cards, updated install section, and extensive CSS/markup changes). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/components/Footer.astro (1)
113-123: Optional polish: keep the badge text on one line.At narrow widths,
20k+ membersmay wrap.white-space: nowrap;keeps the tag visually compact.🎨 Suggested tweak
.footer-tag { display: inline-block; margin-left: 0.4rem; padding: 0.1rem 0.4rem; font-size: 0.7rem; font-weight: 600; color: `#e3e3e3`; background: rgba(255, 255, 255, 0.08); border-radius: 4px; vertical-align: middle; + white-space: nowrap; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/components/Footer.astro` around lines 113 - 123, The .footer-tag CSS rule is allowing the badge text to wrap at narrow widths; update the .footer-tag style (in Footer.astro) to include "white-space: nowrap" so the badge text stays on a single line (optionally also add "overflow: hidden; text-overflow: ellipsis;" if you want to truncate long text gracefully).src/pages/index.astro (3)
279-286: Clipboard fallback could benefit from user feedback.The fallback selects text but doesn't notify the user. Consider showing a brief tooltip or changing the button state to indicate "text selected, press Ctrl+C" so users know the copy operation requires manual completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.astro` around lines 279 - 286, In the catch block for the clipboard write failure (the anonymous .catch handler that currently does range.selectNodeContents(code) and manipulates window.getSelection()), after selecting the code add a clear UI affordance: update the copy button state (e.g., set its text/content to "Selected — press Ctrl+C" or add a CSS class) and trigger a brief tooltip/aria-live announcement to inform the user; ensure you reference the actual copy button element (e.g., copyButton) and/or a tooltip function (e.g., showTooltip) so the UI change is visible and revert it after a short timeout.
73-78: Hardcoded statistics will become stale over time.The star count (22k+) and download count (18M+) are hardcoded. Consider fetching these dynamically or documenting a reminder to update them periodically.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.astro` around lines 73 - 78, The hero-badge currently contains hardcoded stats ("22k+ Stars", "18M+ Downloads") which will become stale; update the template to render dynamic values instead: replace the static text inside the <div class="hero-badge"> with variables/props (e.g., githubStars and downloadCount) and implement a small data-fetch step that queries the GitHub REST API (repo.stargazers_count) and the package registry/downloads endpoint (or a cached build-time script) to populate githubStars and downloadCount; ensure the SVG and surrounding text remain, format numbers with shorteners (k/M) and include sensible caching or build-time fetch to avoid runtime rate limits.
785-789: Negative margin may cause layout fragility.The
margin-top: -2.5remon.section-subtitle-secondarycreates tight coupling with the preceding element's spacing. This could break if.section-subtitle's margin-bottom changes.Consider using a modifier class approach instead
.section-subtitle { text-align: center; color: var(--color-text-secondary); font-size: 1.05rem; - margin-bottom: 3rem; + margin-bottom: 1rem; + } + + .section-subtitle:last-of-type { + margin-bottom: 3rem; } .section-subtitle-secondary { - margin-top: -2.5rem; margin-bottom: 3rem; font-size: 0.95rem; }Alternatively, wrap both subtitles in a container with controlled spacing.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.astro` around lines 785 - 789, Avoid the fragile negative top margin on .section-subtitle-secondary; instead remove margin-top: -2.5rem and implement controlled spacing via a modifier class or wrapper: either add a modifier like .section-subtitle--with-secondary and adjust .section-subtitle's margin-bottom when that modifier is present, or wrap the main and secondary subtitles in a container element (e.g., .subtitle-group) and set explicit gap/margins on that container so spacing is controlled without negative margins; update styles referencing .section-subtitle-secondary and .section-subtitle to use the modifier/container approach.
🤖 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/components/Footer.astro`:
- Around line 113-123: The .footer-tag CSS rule is allowing the badge text to
wrap at narrow widths; update the .footer-tag style (in Footer.astro) to include
"white-space: nowrap" so the badge text stays on a single line (optionally also
add "overflow: hidden; text-overflow: ellipsis;" if you want to truncate long
text gracefully).
In `@src/pages/index.astro`:
- Around line 279-286: In the catch block for the clipboard write failure (the
anonymous .catch handler that currently does range.selectNodeContents(code) and
manipulates window.getSelection()), after selecting the code add a clear UI
affordance: update the copy button state (e.g., set its text/content to
"Selected — press Ctrl+C" or add a CSS class) and trigger a brief
tooltip/aria-live announcement to inform the user; ensure you reference the
actual copy button element (e.g., copyButton) and/or a tooltip function (e.g.,
showTooltip) so the UI change is visible and revert it after a short timeout.
- Around line 73-78: The hero-badge currently contains hardcoded stats ("22k+
Stars", "18M+ Downloads") which will become stale; update the template to render
dynamic values instead: replace the static text inside the <div
class="hero-badge"> with variables/props (e.g., githubStars and downloadCount)
and implement a small data-fetch step that queries the GitHub REST API
(repo.stargazers_count) and the package registry/downloads endpoint (or a cached
build-time script) to populate githubStars and downloadCount; ensure the SVG and
surrounding text remain, format numbers with shorteners (k/M) and include
sensible caching or build-time fetch to avoid runtime rate limits.
- Around line 785-789: Avoid the fragile negative top margin on
.section-subtitle-secondary; instead remove margin-top: -2.5rem and implement
controlled spacing via a modifier class or wrapper: either add a modifier like
.section-subtitle--with-secondary and adjust .section-subtitle's margin-bottom
when that modifier is present, or wrap the main and secondary subtitles in a
container element (e.g., .subtitle-group) and set explicit gap/margins on that
container so spacing is controlled without negative margins; update styles
referencing .section-subtitle-secondary and .section-subtitle to use the
modifier/container approach.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35e91ec2-068c-49d8-a54e-55b09c2d2709
⛔ Files ignored due to path filters (6)
public/images/discord.svgis excluded by!**/*.svgpublic/images/github.svgis excluded by!**/*.svgsrc/assets/homepage/6.pngis excluded by!**/*.pngsrc/assets/homepage/full-app-display.pngis excluded by!**/*.pngsrc/assets/homepage/lyrics-plus.pngis excluded by!**/*.pngsrc/assets/homepage/marketplace.pngis excluded by!**/*.png
📒 Files selected for processing (4)
src/components/Footer.astrosrc/components/icons/DiscordIcon.astrosrc/components/icons/GitHubIcon.astrosrc/pages/index.astro
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/pages/index.astro (1)
77-77: Consider centralizing marketing stats to avoid drift.
22k+,18M+, and200+are duplicated static values. Moving them to a single source (constants/content config) will prevent inconsistent updates across sections.Also applies to: 144-144
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.astro` at line 77, Extract the duplicated marketing numbers ("22k+", "18M+", "200+") from hardcoded strings in index.astro and move them into a single source of truth (e.g., a stats constant or content config object) that the page imports and renders; update the two occurrences (the header marketing line and the other occurrence noted) to reference the new constants instead of literal strings so future updates change in one place (create/modify a module like stats/constants or the page frontmatter export and replace the inline literals in index.astro with those variables).
🤖 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/pages/index.astro`:
- Around line 74-76: The decorative SVG icons in src/pages/index.astro (the
<svg> elements containing the star polygon and the other two similar icons)
should be marked as non-informative for assistive tech: add aria-hidden="true"
and focusable="false" attributes to each decorative <svg> (e.g., the SVG that
contains the <polygon points="12 2 15.09 8.26 ..."> and the other two SVG
occurrences) so screen readers ignore them; apply the same attribute additions
to the other SVG instances mentioned (the ones at the other occurrences
referenced in the comment).
- Around line 101-111: The hero showcase currently sets loading="eager" for
every Image in the heroShowcase map, which hurts initial render; update the map
callback (heroShowcase.map((item, index) => ...)) to only set loading="eager"
for the primary card (e.g., index === 0) and use loading="lazy" (or omit the
prop) for all other images in the Image component inside the hero-showcase-card
so only the first image is eager-loaded.
---
Nitpick comments:
In `@src/pages/index.astro`:
- Line 77: Extract the duplicated marketing numbers ("22k+", "18M+", "200+")
from hardcoded strings in index.astro and move them into a single source of
truth (e.g., a stats constant or content config object) that the page imports
and renders; update the two occurrences (the header marketing line and the other
occurrence noted) to reference the new constants instead of literal strings so
future updates change in one place (create/modify a module like stats/constants
or the page frontmatter export and replace the inline literals in index.astro
with those variables).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dcb97150-dc42-4ea7-aa52-a6d9df0c3c64
📒 Files selected for processing (2)
src/components/Footer.astrosrc/pages/index.astro
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/pages/index.astro (1)
111-121:⚠️ Potential issue | 🟠 MajorOnly eager-load the primary hero card.
At Line 120, every showcase image is eager-loaded. This can compete with critical resources on first paint.
⚡ Suggested fix
- {heroShowcase.map((item) => ( + {heroShowcase.map((item, index) => ( <div class="hero-showcase-card"> <Image src={item.src} alt={item.alt} widths={[400, 700]} sizes="(max-width: 768px) 280px, 380px" format="webp" quality={80} - loading="eager" + loading={index === 0 ? 'eager' : 'lazy'} + fetchpriority={index === 0 ? 'high' : 'auto'} /> <span class="hero-showcase-label">{item.label}</span> </div> ))}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/index.astro` around lines 111 - 121, The hero showcase currently eager-loads every image inside the heroShowcase.map loop (Image components in elements with class "hero-showcase-card"); change the loading behavior so only the primary/first card uses loading="eager" and all subsequent items use loading="lazy" (e.g., check the map index or a flag and set the Image prop accordingly) to avoid competing with critical resources on first paint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/pages/index.astro`:
- Around line 111-121: The hero showcase currently eager-loads every image
inside the heroShowcase.map loop (Image components in elements with class
"hero-showcase-card"); change the loading behavior so only the primary/first
card uses loading="eager" and all subsequent items use loading="lazy" (e.g.,
check the map index or a flag and set the Image prop accordingly) to avoid
competing with critical resources on first paint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4d04af34-f04b-46b2-86b8-ada3d647d3b7
📒 Files selected for processing (2)
src/components/Footer.astrosrc/pages/index.astro
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.
Summary
GitHubIconandDiscordIconAstro componentsTest plan
Summary by CodeRabbit
New Features
Style