forgejo_admin/westside-landing

Fork 0

fix: Queens color switch + staff reorder + registration logo (#116, #117, #118) #119

Merged

forgejo_admin merged 2 commits from 116-117-118-quick-fixes into main

2026-03-27 10:47:32 +00:00

forgejo_admin commented

2026-03-27 08:36:55 +00:00

Owner

Summary

Batch of three playground-approved quick fixes: Queens color toggle not switching accent to pink, staff page ordering (Abbie Sa below Coach Manny), and registration page using wrong logo.

Changes

src/routes/(public)/+layout.svelte: Replaced <svelte:body class:queens-active> directive with $effect that toggles document.body.classList directly for reliable body class application
src/app.css: Changed .queens-active to body.queens-active for proper specificity overriding :root custom properties; updated .reg-logo to width: 150px, removed border-radius: 50% and border
src/routes/(public)/staff/+page.svelte: Swapped Abbie Sa and Coach Manny article blocks so Abbie appears above Manny, matching playground reference
src/routes/(app)/register/+page.svelte: Replaced logo URL from logo-transparent.png to logo-hero-transparent.png

Test Plan

Tests pass locally (build verified)
Navigate to tryouts/teams/schedule, toggle to Queens -- all red accents turn pink (#e91e8c)
Toggle back to Kings -- accents return to red (#d42026)
Visit /staff -- Abbie Sa appears above Coach Manny
Visit /register -- logo is large hero logo (150px wide, no circular border)
No regressions on other public pages

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #116 -- Queens color switch bug
Closes #117 -- Staff page reorder
Closes #118 -- Registration logo fix
westside-app -- the project this work belongs to

## Summary Batch of three playground-approved quick fixes: Queens color toggle not switching accent to pink, staff page ordering (Abbie Sa below Coach Manny), and registration page using wrong logo. ## Changes - `src/routes/(public)/+layout.svelte`: Replaced `<svelte:body class:queens-active>` directive with `$effect` that toggles `document.body.classList` directly for reliable body class application - `src/app.css`: Changed `.queens-active` to `body.queens-active` for proper specificity overriding `:root` custom properties; updated `.reg-logo` to `width: 150px`, removed `border-radius: 50%` and border - `src/routes/(public)/staff/+page.svelte`: Swapped Abbie Sa and Coach Manny article blocks so Abbie appears above Manny, matching playground reference - `src/routes/(app)/register/+page.svelte`: Replaced logo URL from `logo-transparent.png` to `logo-hero-transparent.png` ## Test Plan - [ ] Tests pass locally (build verified) - [ ] Navigate to tryouts/teams/schedule, toggle to Queens -- all red accents turn pink (#e91e8c) - [ ] Toggle back to Kings -- accents return to red (#d42026) - [ ] Visit /staff -- Abbie Sa appears above Coach Manny - [ ] Visit /register -- logo is large hero logo (150px wide, no circular border) - [ ] No regressions on other public pages ## Review Checklist - [ ] Passed automated review-fix loop - [ ] No secrets committed - [ ] No unnecessary file changes - [ ] Commit messages are descriptive ## Related Notes - Closes #116 -- Queens color switch bug - Closes #117 -- Staff page reorder - Closes #118 -- Registration logo fix - `westside-app` -- the project this work belongs to

forgejo_admin added 1 commit

2026-03-27 08:36:55 +00:00

fix: Queens color switch + staff reorder + registration logo (#116 , #117 , #118 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

c8579ef94f

- Replace <svelte:body class:> with $effect for reliable body class toggle
- Update CSS selector to body.queens-active for proper specificity
- Swap Abbie Sa above Coach Manny to match playground order
- Replace small circular logo with logo-hero-transparent.png (150px, no border)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin added 1 commit

2026-03-27 08:38:18 +00:00

fix: add cleanup to queens-active $effect to prevent class leak

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

97379b0b23

When navigating from public to app routes, the queens-active class
on body would persist since $effect doesn't auto-clean DOM mutations.
Return a teardown function to remove the class on layout unmount.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-27 08:38:41 +00:00

Author

Owner

QA Review -- PR #119

Fix 1: Queens color switch (#116)

Replaced <svelte:body class:queens-active> with $effect + document.body.classList -- correct approach for reliable DOM class toggling in Svelte 5
CSS selector updated to body.queens-active for proper specificity over :root custom properties -- correct
Found during review: $effect did not return a cleanup function, meaning the queens-active class would leak onto <body> when navigating from public to app routes. Fixed in commit 97379b0 -- added teardown return to remove the class on layout unmount.

Fix 2: Staff page reorder (#117)

Abbie Sa and Coach Manny article blocks swapped -- Abbie now appears above Manny, matching the playground reference at westside-playground/staff.html
Content is identical, only order changed -- no regressions

Fix 3: Registration logo (#118)

Logo URL changed from logo-transparent.png to logo-hero-transparent.png -- matches playground reference
Alt text improved from "Westside" to "Westside Kings & Queens"
CSS updated: removed border-radius: 50% and border, set width: 150px with height: auto -- matches spec

Checklist

Build passes (npm run build clean)
No secrets committed
No unnecessary file changes (4 files, +32/-22)
No new dependencies added
No API/data-fetching changes
Commit messages are descriptive
Cleanup issue caught and fixed during review

VERDICT: APPROVE

## QA Review -- PR #119 ### Fix 1: Queens color switch (#116) - Replaced `<svelte:body class:queens-active>` with `$effect` + `document.body.classList` -- correct approach for reliable DOM class toggling in Svelte 5 - CSS selector updated to `body.queens-active` for proper specificity over `:root` custom properties -- correct - **Found during review:** `$effect` did not return a cleanup function, meaning the `queens-active` class would leak onto `<body>` when navigating from public to app routes. **Fixed in commit 97379b0** -- added teardown return to remove the class on layout unmount. ### Fix 2: Staff page reorder (#117) - Abbie Sa and Coach Manny article blocks swapped -- Abbie now appears above Manny, matching the playground reference at `westside-playground/staff.html` - Content is identical, only order changed -- no regressions ### Fix 3: Registration logo (#118) - Logo URL changed from `logo-transparent.png` to `logo-hero-transparent.png` -- matches playground reference - Alt text improved from "Westside" to "Westside Kings & Queens" - CSS updated: removed `border-radius: 50%` and `border`, set `width: 150px` with `height: auto` -- matches spec ### Checklist - [x] Build passes (`npm run build` clean) - [x] No secrets committed - [x] No unnecessary file changes (4 files, +32/-22) - [x] No new dependencies added - [x] No API/data-fetching changes - [x] Commit messages are descriptive - [x] Cleanup issue caught and fixed during review **VERDICT: APPROVE**

forgejo_admin commented

2026-03-27 10:47:01 +00:00

Author

Owner

PR #119 Review

DOMAIN REVIEW

Tech stack: SvelteKit 5 (runes), pure CSS custom properties, adapter-static. No Tailwind (correct per convention).

Queens color switch ($effect + body.queens-active)

The core fix replaces the <svelte:body class:queens-active={...}> directive with an imperative $effect that toggles document.body.classList. This is the correct approach -- the Svelte 5 class: directive on <svelte:body> has known limitations with CSS custom property inheritance when the selector needs to override :root.

Specificity analysis: :root custom properties (specificity 0,1,0 on html) are inherited by body. The body.queens-active selector (specificity 0,1,1) sets --color-red, --color-red-rgb, and --color-red-hover directly on the body element. Because custom properties inherit down the DOM, all descendants will see the overridden values. This is correct and reliable.

The $effect cleanup function (return () => { document.body.classList.remove('queens-active') }) correctly removes the class when the (public) layout unmounts (e.g., navigating to (app) routes). This prevents the pink color from leaking into the registration/auth pages. Good defensive coding.

Note: the playground CSS uses .queens-active (without body. prefix) while the app uses body.queens-active. The app version is actually more correct -- the playground worked because its JS always targets document.body, so the class was always on body. Adding the body type selector makes the intent explicit and adds specificity.

Staff reorder (Abbie Sa above Coach Manny)

Pure block swap -- no content added, removed, or modified. Both article blocks are byte-identical to their pre-swap versions. The order now matches the playground reference (staff.html lines 146-170: Abbie Sa at line 146, Coach Manny at line 159).

Registration logo

URL changed from logo-transparent.png to logo-hero-transparent.png -- matches playground reference (register.html line 43 uses logo-hero.png, the MinIO asset equivalent is logo-hero-transparent.png)
Alt text improved from "Westside" to "Westside Kings & Queens" -- more descriptive, better accessibility
CSS: width: 60px -> 150px, height: 60px -> auto, border-radius: 50% removed, border: 3px solid var(--color-red) -> border: none, object-fit: cover -> contain -- matches playground .reg-logo styling

Minor discrepancy: playground CSS has display: block; margin: 0 auto var(--space-md); on .reg-logo that the app version does not. The app keeps margin-bottom: 1rem (functionally equivalent to --space-md in this design system) and relies on the parent .reg-header { text-align: center } for centering instead of display: block; margin: 0 auto. Both approaches center the image correctly given the context -- this is fine.

BLOCKERS

None.

No new functionality was added -- these are three visual/content fixes (CSS selector specificity, HTML block reorder, image URL swap). No test coverage blocker applies.
No user input handling was changed.
No secrets or credentials in the diff.
No DRY violations introduced.

Regarding the project's zero test infrastructure: westside-app has no test framework configured (package.json has no vitest/jest/playwright). This is a pre-existing condition, not introduced by this PR. These are CSS/content-only changes that would require visual regression testing anyway, which the manual Test Plan in the PR body adequately covers.

NITS

Playground CSS drift: The playground shared/style.css line 2904 still uses .queens-active while the app now uses body.queens-active. Consider updating the playground to stay in sync, so future copy-paste operations don't regress this fix. (Separate issue/PR scope.)
reg-logo missing display: block: The playground has display: block; margin: 0 auto on .reg-logo for explicit centering. The app relies on text-align: center on the parent. Both work, but the playground approach is more defensive if the image is ever moved outside .reg-header. Very minor.

SOP COMPLIANCE

Branch named after issue: 116-117-118-quick-fixes references all three issue numbers
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes sections all present
Related section references issues: Closes #116, #117, #118 with clear descriptions
Related references plan slug: No plan slug referenced -- but these are standalone bug/content fixes, not plan-phase work. Acceptable per feedback_todos_plan_pipeline.md ("Bugs/fixes go straight to Forgejo issue + board").
No secrets committed
No unnecessary file changes: exactly 4 files touched, all directly related to the three issues
Commit messages are descriptive (PR title follows conventional commits: fix: Queens color switch + staff reorder + registration logo)

PROCESS OBSERVATIONS

Batch PR for related quick fixes: Grouping three small visual fixes into one PR is pragmatic. All three are content/CSS-only with zero behavioral overlap, so the blast radius is contained. Good use of batch fixing for low-risk changes.
Change failure risk: Low. CSS specificity fix is well-understood, block swap is zero-logic, logo URL change is a single attribute. Manual visual verification per the Test Plan is the appropriate gate.
Deployment frequency: This unblocks three issues in a single deploy cycle. Positive DORA signal.
No test infrastructure: westside-app still has no test framework. For a static landing site this is acceptable near-term, but as the app grows (registration flow with Keycloak, Stripe payments), a testing strategy should be prioritized.

VERDICT: APPROVED

## PR #119 Review ### DOMAIN REVIEW **Tech stack:** SvelteKit 5 (runes), pure CSS custom properties, adapter-static. No Tailwind (correct per convention). **Queens color switch (`$effect` + `body.queens-active`)** The core fix replaces the `<svelte:body class:queens-active={...}>` directive with an imperative `$effect` that toggles `document.body.classList`. This is the correct approach -- the Svelte 5 `class:` directive on `<svelte:body>` has known limitations with CSS custom property inheritance when the selector needs to override `:root`. Specificity analysis: `:root` custom properties (specificity 0,1,0 on `html`) are inherited by `body`. The `body.queens-active` selector (specificity 0,1,1) sets `--color-red`, `--color-red-rgb`, and `--color-red-hover` directly on the body element. Because custom properties inherit down the DOM, all descendants will see the overridden values. This is correct and reliable. The `$effect` cleanup function (`return () => { document.body.classList.remove('queens-active') }`) correctly removes the class when the `(public)` layout unmounts (e.g., navigating to `(app)` routes). This prevents the pink color from leaking into the registration/auth pages. Good defensive coding. Note: the playground CSS uses `.queens-active` (without `body.` prefix) while the app uses `body.queens-active`. The app version is actually more correct -- the playground worked because its JS always targets `document.body`, so the class was always on `body`. Adding the `body` type selector makes the intent explicit and adds specificity. **Staff reorder (Abbie Sa above Coach Manny)** Pure block swap -- no content added, removed, or modified. Both article blocks are byte-identical to their pre-swap versions. The order now matches the playground reference (`staff.html` lines 146-170: Abbie Sa at line 146, Coach Manny at line 159). **Registration logo** - URL changed from `logo-transparent.png` to `logo-hero-transparent.png` -- matches playground reference (`register.html` line 43 uses `logo-hero.png`, the MinIO asset equivalent is `logo-hero-transparent.png`) - Alt text improved from "Westside" to "Westside Kings & Queens" -- more descriptive, better accessibility - CSS: `width: 60px` -> `150px`, `height: 60px` -> `auto`, `border-radius: 50%` removed, `border: 3px solid var(--color-red)` -> `border: none`, `object-fit: cover` -> `contain` -- matches playground `.reg-logo` styling Minor discrepancy: playground CSS has `display: block; margin: 0 auto var(--space-md);` on `.reg-logo` that the app version does not. The app keeps `margin-bottom: 1rem` (functionally equivalent to `--space-md` in this design system) and relies on the parent `.reg-header { text-align: center }` for centering instead of `display: block; margin: 0 auto`. Both approaches center the image correctly given the context -- this is fine. ### BLOCKERS None. - No new functionality was added -- these are three visual/content fixes (CSS selector specificity, HTML block reorder, image URL swap). No test coverage blocker applies. - No user input handling was changed. - No secrets or credentials in the diff. - No DRY violations introduced. Regarding the project's zero test infrastructure: westside-app has no test framework configured (`package.json` has no vitest/jest/playwright). This is a pre-existing condition, not introduced by this PR. These are CSS/content-only changes that would require visual regression testing anyway, which the manual Test Plan in the PR body adequately covers. ### NITS 1. **Playground CSS drift:** The playground `shared/style.css` line 2904 still uses `.queens-active` while the app now uses `body.queens-active`. Consider updating the playground to stay in sync, so future copy-paste operations don't regress this fix. (Separate issue/PR scope.) 2. **`reg-logo` missing `display: block`:** The playground has `display: block; margin: 0 auto` on `.reg-logo` for explicit centering. The app relies on `text-align: center` on the parent. Both work, but the playground approach is more defensive if the image is ever moved outside `.reg-header`. Very minor. ### SOP COMPLIANCE - [x] Branch named after issue: `116-117-118-quick-fixes` references all three issue numbers - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related Notes sections all present - [x] Related section references issues: Closes #116, #117, #118 with clear descriptions - [ ] Related references plan slug: No plan slug referenced -- but these are standalone bug/content fixes, not plan-phase work. Acceptable per `feedback_todos_plan_pipeline.md` ("Bugs/fixes go straight to Forgejo issue + board"). - [x] No secrets committed - [x] No unnecessary file changes: exactly 4 files touched, all directly related to the three issues - [x] Commit messages are descriptive (PR title follows conventional commits: `fix: Queens color switch + staff reorder + registration logo`) ### PROCESS OBSERVATIONS - **Batch PR for related quick fixes:** Grouping three small visual fixes into one PR is pragmatic. All three are content/CSS-only with zero behavioral overlap, so the blast radius is contained. Good use of batch fixing for low-risk changes. - **Change failure risk:** Low. CSS specificity fix is well-understood, block swap is zero-logic, logo URL change is a single attribute. Manual visual verification per the Test Plan is the appropriate gate. - **Deployment frequency:** This unblocks three issues in a single deploy cycle. Positive DORA signal. - **No test infrastructure:** westside-app still has no test framework. For a static landing site this is acceptable near-term, but as the app grows (registration flow with Keycloak, Stripe payments), a testing strategy should be prioritized. ### VERDICT: APPROVED