Credits page, and topbar change - #236
Conversation
Made the credits page still needs update with peoples contribution or titles however we want it, and made the topbar a bit bigger to 90px
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughPR adds a new credits page with rolling animation capability, resizes topbar branding elements and increases spacing, restructures login footer layout with flexbox, and adds a credits link to the login page footer. ChangesTopbar Visual Refinement
Credits Feature Addition
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsTimed out fetching pipeline failures after 30000ms 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. Review rate limit: 0/1 reviews remaining, refill in 54 minutes and 18 seconds.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 (2)
rocky-interface/src/routes/credits/+page.svelte (2)
5-9: ⚡ Quick winAdd an optional
urlfield to theCredittype instead of hard-coding a name check.The template currently determines whether to render a link by comparing
credit.nameto the string'Kent State University'(line 114). This is fragile: a name change silently drops the link, and adding a URL to any future asset credit requires modifying the template rather than just the data.♻️ Proposed refactor
type Credit = { name: string; title: string; variant?: 'navy' | 'gold'; + url?: string; };const assetCredits: Credit[] = [ { name: 'Kent State University', title: 'Main login image and Kent State logo', variant: 'gold', + url: 'https://www.kent.edu/' }, { name: 'Archie Horne', title: 'Profile pictures and Rocky logo', variant: 'navy' } ];- {`#if` credit.name === 'Kent State University'} - <a href="https://www.kent.edu/">kent.edu</a> - {/if} + {`#if` credit.url} + <a href={credit.url}>{credit.url.replace(/^https?:\/\//, '').replace(/\/$/, '')}</a> + {/if}Also applies to: 24-31, 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocky-interface/src/routes/credits/`+page.svelte around lines 5 - 9, The Credit type currently lacks a url field and the template uses a brittle string check on credit.name to decide whether to render an anchor; add an optional url?: string to the Credit type (type Credit) and update the template rendering logic (where it checks credit.name === 'Kent State University') to instead render an <a> when credit.url is present (using credit.url as href) and fallback to plain text when absent; update any data fixtures/arrays that should be links to include the url property.
85-85: 💤 Low value
aria-live="polite"on.credits-stageis effectively inert.The live region fires when DOM mutations occur inside the element. However, toggling
isRollingonly adds/removes thecredits-rollingclass on the ancestor.credits-shell— no content inside.credits-stagechanges. Screen readers will never receive an announcement for the roll state change from this attribute.If the intent is to announce the rolling state to AT users, a dedicated visually-hidden status element updated alongside
isRollingwould work:- <div class="credits-stage" aria-live="polite"> + <div class="credits-stage">+ <span class="sr-only" aria-live="polite" aria-atomic="true"> + {isRolling ? 'Credits are rolling.' : ''} + </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocky-interface/src/routes/credits/`+page.svelte at line 85, The .credits-stage element uses aria-live="polite" but nothing inside it changes when isRolling toggles (only the ancestor .credits-shell class changes), so screen readers won't be notified; add a dedicated, visually-hidden status node (e.g., a span with role="status" or aria-live="polite") inside the component and update its text whenever the isRolling state changes (update the same reactive block or handler that toggles isRolling), ensuring the element is visually hidden but present in the DOM so AT can announce "rolling" / "paused" (reference isRolling, .credits-stage and .credits-shell to locate where to insert and update the status node).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rocky-interface/src/lib/styles/routes/modules/credits-view.css`:
- Around line 392-398: The reduced-motion rule currently sets
animation-duration: 1ms on .credit-card and .credits-rolling .credits-list
which, combined with the credits-roll animation's forwards fill-mode, snaps
elements to the final (off-screen) state; change the rule to disable animations
entirely for these selectors by using animation: none (and keep transition:
none) so decorative credit-roll animations are removed for
prefers-reduced-motion users instead of forcing a near-zero duration that jumps
to translateY(-106%); update the selectors .credit-card and .credits-rolling
.credits-list in the prefers-reduced-motion block accordingly.
---
Nitpick comments:
In `@rocky-interface/src/routes/credits/`+page.svelte:
- Around line 5-9: The Credit type currently lacks a url field and the template
uses a brittle string check on credit.name to decide whether to render an
anchor; add an optional url?: string to the Credit type (type Credit) and update
the template rendering logic (where it checks credit.name === 'Kent State
University') to instead render an <a> when credit.url is present (using
credit.url as href) and fallback to plain text when absent; update any data
fixtures/arrays that should be links to include the url property.
- Line 85: The .credits-stage element uses aria-live="polite" but nothing inside
it changes when isRolling toggles (only the ancestor .credits-shell class
changes), so screen readers won't be notified; add a dedicated, visually-hidden
status node (e.g., a span with role="status" or aria-live="polite") inside the
component and update its text whenever the isRolling state changes (update the
same reactive block or handler that toggles isRolling), ensuring the element is
visually hidden but present in the DOM so AT can announce "rolling" / "paused"
(reference isRolling, .credits-stage and .credits-shell to locate where to
insert and update the status node).
🪄 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: 9b01a547-4bdb-4c38-bc17-6977ccf0ca96
📒 Files selected for processing (7)
rocky-interface/src/lib/styles/components/components.cssrocky-interface/src/lib/styles/components/modules/topbar.cssrocky-interface/src/lib/styles/foundation/tokens.cssrocky-interface/src/lib/styles/routes/modules/credits-view.cssrocky-interface/src/lib/styles/routes/modules/login-view.cssrocky-interface/src/routes/credits/+page.svelterocky-interface/src/routes/login/+page.svelte
| @media (prefers-reduced-motion: reduce) { | ||
| .credit-card, | ||
| .credits-rolling .credits-list { | ||
| transition: none; | ||
| animation-duration: 1ms; | ||
| } | ||
| } |
There was a problem hiding this comment.
animation-duration: 1ms with forwards fill-mode silently pushes all credits off-screen for reduced-motion users.
Because credits-roll uses animation-fill-mode: forwards (via the 34s linear forwards shorthand on line 379), setting animation-duration: 1ms causes the element to immediately jump to the end state — translateY(-106%) — putting all content above the viewport. A user who has reduced motion enabled and clicks "Roll Credits" will see every credit card disappear with no visible motion, and the page looks blank until they click "Reset Credits".
When setting an animation's duration to 0 (or near-zero) with animation-fill-mode: forwards, the element snaps to the final keyframe state, so the default state of that element must be considered manually. Since this code has no animationend listener, using animation: none is safe and is the correct fix. Most decorative effects such as slides and parallax can simply be removed for people who prefer reduced motion.
🛡️ Proposed fix
`@media` (prefers-reduced-motion: reduce) {
- .credit-card,
- .credits-rolling .credits-list {
- transition: none;
- animation-duration: 1ms;
- }
+ .credit-card {
+ transition: none;
+ }
+
+ .credits-rolling .credits-list {
+ animation: none;
+ }
}📝 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.
| @media (prefers-reduced-motion: reduce) { | |
| .credit-card, | |
| .credits-rolling .credits-list { | |
| transition: none; | |
| animation-duration: 1ms; | |
| } | |
| } | |
| `@media` (prefers-reduced-motion: reduce) { | |
| .credit-card { | |
| transition: none; | |
| } | |
| .credits-rolling .credits-list { | |
| animation: none; | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@rocky-interface/src/lib/styles/routes/modules/credits-view.css` around lines
392 - 398, The reduced-motion rule currently sets animation-duration: 1ms on
.credit-card and .credits-rolling .credits-list which, combined with the
credits-roll animation's forwards fill-mode, snaps elements to the final
(off-screen) state; change the rule to disable animations entirely for these
selectors by using animation: none (and keep transition: none) so decorative
credit-roll animations are removed for prefers-reduced-motion users instead of
forcing a near-zero duration that jumps to translateY(-106%); update the
selectors .credit-card and .credits-rolling .credits-list in the
prefers-reduced-motion block accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rocky-interface/src/lib/styles/components/components.css (1)
52-60:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDuplicate mobile media queries produce dead
height: 28pxon.ksu-logoTwo
@media (max-width: 768px)blocks both target.ksu-logo. The block at lines 52–60 setsheight: 28px(along withposition: static,transform: none, etc.), but the block at lines 101–103 — appearing later in the cascade — setsheight: 42px, silently overriding it. Theheight: 28pxrule is effectively dead code.Consolidate into a single mobile block for
.ksu-logo:🔧 Proposed fix — merge the two mobile rules
-/* Lines 52-60 */ `@media` (max-width: 768px) { .ksu-logo { - height: 28px; - position: static; - transform: none; - margin-left: auto; - margin-right: var(--space-sm); + height: 42px; + position: static; + transform: none; + margin-left: auto; + margin-right: var(--space-sm); } } -/* Lines 101-103 (in the second media block) — remove the duplicate */ - .ksu-logo { - height: 42px; - }Also applies to: 101-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@rocky-interface/src/lib/styles/components/components.css` around lines 52 - 60, There are two duplicate `@media` (max-width: 768px) rules targeting .ksu-logo which leaves height: 28px dead because a later block sets height: 42px; consolidate them by merging the properties for .ksu-logo into a single `@media` (max-width: 768px) block (remove the duplicate block), ensure the final height value is the intended one and include position, transform, and margin rules together so the cascade/order no longer silently overrides height for .ksu-logo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@rocky-interface/src/lib/styles/components/components.css`:
- Around line 44-50: The .ksu-logo rule uses position:absolute and left:85%
which removes it from the flex flow and causes overlap; change the rule for
.ksu-logo (in components.css) to participate in the topbar flex layout instead:
remove position:absolute and left, and rely on flex alignment (e.g., use
margin-left:auto on .ksu-logo or the existing topbar pattern) so the logo is
pushed to the right without breaking the layout; keep height (56px) and
width:auto and add align-self:center or appropriate vertical alignment if needed
to match .brand-text.
---
Outside diff comments:
In `@rocky-interface/src/lib/styles/components/components.css`:
- Around line 52-60: There are two duplicate `@media` (max-width: 768px) rules
targeting .ksu-logo which leaves height: 28px dead because a later block sets
height: 42px; consolidate them by merging the properties for .ksu-logo into a
single `@media` (max-width: 768px) block (remove the duplicate block), ensure the
final height value is the intended one and include position, transform, and
margin rules together so the cascade/order no longer silently overrides height
for .ksu-logo.
🪄 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: 93640d48-efd7-4c31-ae41-827a91ca640c
📒 Files selected for processing (1)
rocky-interface/src/lib/styles/components/components.css
2 participants
Made the credits page still needs update with peoples contribution or titles however we want it, and made the topbar a bit bigger to 90px
Summary by CodeRabbit
Release Notes
New Features
Style