fabi/Mangalord
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix: address skeleton/nav-progress review findings

Browse Source 
Correctness:
- MangaGridSkeleton: covers rendered 0px tall because height="0" is a
  definite height that defeats aspect-ratio. Skeleton gains an `aspectRatio`
  prop (height stays auto) so the 2/3 cover box is reserved and the grid no
  longer shifts when real covers load.
- Move `.manga-card` layout into global tokens.css (it was scoped to
  MangaCard, so the skeleton's cards inherited none of it). Both consumers
  now share one definition.
- NavProgress: rebuilt as an idle/loading/done state machine. It now
  completes to full and fades on settle (was snapping away instantly),
  restarts the trickle when one navigation supersedes another (was parking
  frozen near 90%), animates transform/opacity only with will-change gated to
  the active phase, and hides itself in reader fullscreen.
- Skeleton: text-variant height resolved in one place (no inline style
  shadowing a stylesheet rule); shimmer gradient/animation gated behind
  prefers-reduced-motion: no-preference so reduced motion shows a flat block.

A11y:
- Home loading region announces via visually-hidden text instead of an
  aria-label a live region won't reliably read.

Cleanup:
- Drop the fragile :first-child selector and index-keying ceremony in
  MangaGridSkeleton; simplify Skeleton's style construction.

Tests: adds regression guards for the 0px cover collapse, the aspect-ratio
box, text-height defaulting, and the nav-progress complete/restart phases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
MechaCat02
2026-07-06 20:02:27 +02:00
parent 7bdbe3ce5b
commit 0d9505ce9f
13 changed files with 231 additions and 99 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
backend/Cargo.lock generated
View File

@@ -1558,7 +1558,7 @@ checksum = "c41e0c4fef86961ac6d6f8a82609f55f31b05e4fce149ac5710e439df7619ba4"
[[package]]
name = "mangalord"
version = "0.114.1"
version = "0.114.2"
dependencies = [
"anyhow",
"argon2",

2
backend/Cargo.toml
View File

@@ -1,6 +1,6 @@
[package]
name = "mangalord"
version = "0.114.1"
version = "0.114.2"
edition = "2021"
default-run = "mangalord"

11
frontend/e2e/nav-progress.spec.ts
View File

@@ -132,16 +132,17 @@ test('shows the nav progress bar during a pending navigation and clears it after
const bar = page.getByTestId('nav-progress');
// Idle on a settled page.
await expect(bar).not.toHaveClass(/active/);
await expect(bar).toHaveAttribute('data-phase', 'idle');
// Client-side navigate into the (gated) detail page.
await page.locator(`a[href="/manga/${mangaId}"]`).first().click();
// Bar activates while the detail load is in flight.
await expect(bar).toHaveClass(/active/);
// Bar enters the loading phase while the detail load is in flight.
await expect(bar).toHaveAttribute('data-phase', 'loading');
// Let the load resolve; the detail renders and the bar goes idle again.
// Let the load resolve; the detail renders and the bar settles back to
// idle (via a brief done phase).
releaseDetail();
await expect(page.getByTestId('manga-title')).toHaveText('Berserk');
await expect(bar).not.toHaveClass(/active/);
await expect(bar).toHaveAttribute('data-phase', 'idle');
});

2
frontend/package.json
View File

@@ -1,6 +1,6 @@
{
"name": "mangalord-frontend",
"version": "0.114.1",
"version": "0.114.2",
"private": true,
"type": "module",
"scripts": {

13
frontend/src/lib/components/MangaCard.svelte
View File

@@ -90,17 +90,8 @@
</li>
<style>
.manga-card {
display: flex;
flex-direction: column;
gap: var(--space-2);
list-style: none;
/* Grid items default to min-width: auto which equals the
intrinsic content size — long author / title strings then
push the cell past `1fr`. Forcing 0 lets the column control
the width and the children ellipsize inside it. */
min-width: 0;
}
/* `.manga-card` layout lives in tokens.css so the catalog cards and the
loading skeleton share one definition. */
.cover-link {
display: block;

26
frontend/src/lib/components/MangaGridSkeleton.svelte
View File

@@ -3,33 +3,19 @@
/**
* Placeholder for a manga cover grid while it loads. Reuses the global
* `.manga-grid` / `.manga-card` shapes (single-sourced in tokens.css) so
* it lines up pixel-for-pixel with the real grid at every breakpoint and
* content swaps in without a layout shift.
* `.manga-grid` and `.manga-card` layout (both single-sourced in
* tokens.css) so it lines up with the real grid at every breakpoint and
* content swaps in without a layout shift. The cover reserves its 2/3 box
* via `aspect-ratio` (height stays auto), matching MangaCard's cover.
*/
let { count = 12 }: { count?: number } = $props();
const cards = $derived(Array.from({ length: count }, (_, i) => i));
</script>
<ul class="manga-grid" data-testid="manga-grid-skeleton" aria-hidden="true">
{#each cards as i (i)}
{#each Array(count) as _}
<li class="manga-card">
<Skeleton height="0" radius="md" />
<Skeleton aspectRatio="2 / 3" radius="md" />
<Skeleton variant="text" width="80%" radius="sm" />
</li>
{/each}
</ul>
<style>
/* Reserve the 2/3 cover box the same way MangaCard does. `height: 0` on
the primitive plus aspect-ratio here gives the cover its shape without
hard-coding pixels. */
.manga-card :global(.skeleton:first-child) {
aspect-ratio: 2 / 3;
}
.manga-card :global(.skeleton.text) {
margin-top: var(--space-1);
}
</style>

12
frontend/src/lib/components/MangaGridSkeleton.svelte.test.ts
View File

@@ -28,4 +28,16 @@ describe('MangaGridSkeleton', () => {
render(MangaGridSkeleton);
expect(screen.getByTestId('manga-grid-skeleton').getAttribute('aria-hidden')).toBe('true');
});
it('reserves each cover as a 2/3 aspect box, not a collapsed zero-height one', () => {
render(MangaGridSkeleton, { props: { count: 1 } });
const card = screen
.getByTestId('manga-grid-skeleton')
.querySelector('.manga-card') as HTMLElement;
const cover = card.firstElementChild as HTMLElement;
expect(cover.style.aspectRatio).toBe('2 / 3');
// A definite height:0 would make aspect-ratio a no-op and collapse
// the cover — the exact layout-shift regression this guards.
expect(cover.style.height).toBe('');
});
});

97
frontend/src/lib/components/NavProgress.svelte
View File

@@ -2,57 +2,110 @@
import { navigating } from '$app/stores';
/**
* Thin top-of-viewport progress bar that shows during client-side
* navigation. Most data routes run their `load` with `ssr = false`, so
* SvelteKit keeps the previous page on screen while the next one's data
* resolves — with no feedback. This bar is that feedback: it trickles
* toward the right edge while a navigation is pending and fades out once
* it settles.
* Thin top-of-viewport progress bar shown during client-side navigation.
* Most data routes run their `load` with `ssr = false`, so SvelteKit holds
* the previous page on screen while the next one's data resolves — with no
* feedback. This bar is that feedback: it trickles toward the right edge
* while a navigation is pending, snaps to full and fades out once it
* settles.
*
* Decorative (`aria-hidden`). Under the global prefers-reduced-motion
* override the trickle animation is disabled, so the `.active` rule keeps
* a static visible bar during navigation instead of an empty sliver.
* Decorative (`aria-hidden`). Animates `transform`/`opacity` only (both
* compositor-friendly), and drops `will-change` when idle. Under
* prefers-reduced-motion the trickle keyframe is neutralized by the global
* override, leaving the `.loading` base transform as a static visible bar.
*/
const active = $derived(!!$navigating);
// idle: hidden. loading: trickling. done: snap to full, then fade to idle.
let phase = $state<'idle' | 'loading' | 'done'>('idle');
// Bumped whenever a *new* navigation begins. Keying the element on it
// remounts the bar so the trickle animation restarts even when one
// navigation supersedes another (the store stays truthy the whole time,
// just swapping Navigation objects).
let run = $state(0);
// Plain locals (not $state) so reading them in the effect doesn't make the
// effect depend on its own writes. `runCount` mirrors `run` so we can bump
// it without reactively reading the `run` state.
let runCount = 0;
let wasNavigating = false;
let doneTimer: ReturnType<typeof setTimeout> | undefined;
$effect(() => {
const isNavigating = !!$navigating;
if (isNavigating) {
clearTimeout(doneTimer);
run = ++runCount;
phase = 'loading';
} else if (wasNavigating) {
phase = 'done';
doneTimer = setTimeout(() => {
phase = 'idle';
}, 260);
}
wasNavigating = isNavigating;
return () => clearTimeout(doneTimer);
});
</script>
<div class="nav-progress" class:active data-testid="nav-progress" aria-hidden="true"></div>
{#key run}
<div
class="nav-progress"
class:loading={phase === 'loading'}
class:done={phase === 'done'}
data-testid="nav-progress"
data-phase={phase}
aria-hidden="true"
></div>
{/key}
<style>
.nav-progress {
position: fixed;
top: 0;
left: 0;
width: 100%;
height: 3px;
width: 0;
background: var(--primary);
z-index: var(--z-nav-progress);
transform: scaleX(0);
transform-origin: left;
opacity: 0;
/* Fade out on completion; the trickle (width) is handled by the
keyframe below while active. */
transition: opacity 200ms ease-out;
pointer-events: none;
will-change: width, opacity;
}
.nav-progress.active {
.nav-progress.loading {
opacity: 1;
/* Base (non-animated) width — this is what reduced-motion users see,
/* Base (non-animated) target — this is what reduced-motion users see,
and the keyframe's end state, so disabling the animation still
leaves a clearly visible bar. */
width: 90%;
transform: scaleX(0.9);
animation: nav-progress-trickle 8s cubic-bezier(0.1, 0.7, 0.3, 1) forwards;
will-change: transform;
}
.nav-progress.done {
opacity: 0;
transform: scaleX(1);
/* Complete to full quickly, then fade out. */
transition:
transform 120ms ease-out,
opacity 200ms ease-out 120ms;
}
/* The reader hides all its own chrome in fullscreen; the progress bar
would be the one visible element left, so hide it too. */
:global(html[data-reader-fullscreen='true']) .nav-progress {
display: none;
}
@keyframes nav-progress-trickle {
0% {
width: 0;
transform: scaleX(0);
}
50% {
width: 65%;
transform: scaleX(0.65);
}
100% {
width: 90%;
transform: scaleX(0.9);
}
}
</style>

62
frontend/src/lib/components/NavProgress.svelte.test.ts
View File

@@ -1,4 +1,5 @@
import { describe, it, expect, afterEach, vi } from 'vitest';
import { flushSync } from 'svelte';
import { render, screen, cleanup } from '@testing-library/svelte';
// A controllable stand-in for SvelteKit's `navigating` store: `null` when
@@ -27,6 +28,8 @@ vi.mock('$app/stores', () => ({ navigating }));
import NavProgress from './NavProgress.svelte';
const nav = (path: string) => ({ from: null, to: { url: new URL(`http://x${path}`) } });
afterEach(() => {
cleanup();
navigating.set(null);
@@ -40,25 +43,58 @@ describe('NavProgress', () => {
expect(bar.getAttribute('aria-hidden')).toBe('true');
});
it('is inactive when not navigating', () => {
it('is idle when not navigating', () => {
render(NavProgress);
expect(screen.getByTestId('nav-progress').classList.contains('active')).toBe(false);
const bar = screen.getByTestId('nav-progress');
expect(bar.dataset.phase).toBe('idle');
expect(bar.classList.contains('loading')).toBe(false);
});
it('becomes active while a navigation is pending', async () => {
it('enters the loading phase while a navigation is pending', () => {
render(NavProgress);
navigating.set({ from: null, to: { url: new URL('http://x/manga/1') } });
await Promise.resolve();
expect(screen.getByTestId('nav-progress').classList.contains('active')).toBe(true);
navigating.set(nav('/manga/1'));
flushSync();
const bar = screen.getByTestId('nav-progress');
expect(bar.dataset.phase).toBe('loading');
expect(bar.classList.contains('loading')).toBe(true);
});
it('returns to inactive once navigation settles', async () => {
it('snaps to done then returns to idle once navigation settles', () => {
vi.useFakeTimers();
try {
render(NavProgress);
navigating.set(nav('/manga/1'));
flushSync();
expect(screen.getByTestId('nav-progress').dataset.phase).toBe('loading');
navigating.set(null);
flushSync();
// Completes to full and fades, rather than snapping away instantly.
expect(screen.getByTestId('nav-progress').dataset.phase).toBe('done');
vi.advanceTimersByTime(300);
flushSync();
expect(screen.getByTestId('nav-progress').dataset.phase).toBe('idle');
} finally {
vi.useRealTimers();
}
});
it('restarts the bar when a new navigation supersedes a pending one', () => {
render(NavProgress);
navigating.set({ from: null, to: { url: new URL('http://x/manga/1') } });
await Promise.resolve();
expect(screen.getByTestId('nav-progress').classList.contains('active')).toBe(true);
navigating.set(null);
await Promise.resolve();
expect(screen.getByTestId('nav-progress').classList.contains('active')).toBe(false);
navigating.set(nav('/manga/1'));
flushSync();
const first = screen.getByTestId('nav-progress');
// A second navigation begins before the first settles — the store
// stays truthy the whole time, just swapping objects.
navigating.set(nav('/manga/2'));
flushSync();
const second = screen.getByTestId('nav-progress');
expect(second.dataset.phase).toBe('loading');
// {#key run} remounts the element so the trickle animation replays
// rather than staying parked at its previous width.
expect(second).not.toBe(first);
});
});

56
frontend/src/lib/components/Skeleton.svelte
View File

@@ -5,19 +5,25 @@
* out of the accessibility tree; the loading state should be announced by
* the container (e.g. `role="status"`).
*
* The shimmer freezes to a solid `--surface-elevated` block under the
* global `prefers-reduced-motion` override (see tokens.css), which is the
* intended reduced-motion appearance.
* Under prefers-reduced-motion the shimmer gradient/animation is dropped
* entirely, leaving a flat `--surface-elevated` block.
*/
let {
width = '100%',
height,
aspectRatio,
radius = 'md',
variant = 'block',
testid
}: {
width?: string;
height?: string;
/**
* `aspect-ratio` for the box (e.g. `'2 / 3'`). Lets a fluid-width
* placeholder reserve its shape without a hard-coded height — the
* height stays `auto` so the ratio actually applies.
*/
aspectRatio?: string;
/** Corner radius, mapped to the design-token scale. */
radius?: 'sm' | 'md' | 'lg' | 'pill';
/** `circle` forces a pill radius (for avatars / dots). */
@@ -27,38 +33,40 @@
const radiusClass = $derived(variant === 'circle' ? 'radius-pill' : `radius-${radius}`);
// A text line gets a sensible default height so callers don't have to; an
// explicit `height` still wins. Resolving it here (rather than in CSS)
// keeps a single source of truth for the box height and avoids an inline
// style silently shadowing a stylesheet rule.
const resolvedHeight = $derived(height ?? (variant === 'text' ? '0.9em' : undefined));
const style = $derived(
[`width: ${width};`, height ? `height: ${height};` : null].filter(Boolean).join(' ')
`width: ${width};` +
(resolvedHeight ? ` height: ${resolvedHeight};` : '') +
(aspectRatio ? ` aspect-ratio: ${aspectRatio};` : '')
);
</script>
<span
class="skeleton {radiusClass}"
class:text={variant === 'text'}
data-testid={testid}
aria-hidden="true"
{style}
></span>
<span class="skeleton {radiusClass}" data-testid={testid} aria-hidden="true" {style}></span>
<style>
.skeleton {
display: block;
/* Static base so the reduced-motion freeze reads as a clean block
rather than a stalled mid-keyframe gradient. */
/* Flat base — this is the whole appearance under reduced motion. */
background-color: var(--surface-elevated);
background-image: linear-gradient(
90deg,
var(--surface) 0%,
var(--surface-elevated) 50%,
var(--surface) 100%
);
background-size: 200% 100%;
background-repeat: no-repeat;
animation: skeleton-shimmer 1.2s ease-in-out infinite;
}
.skeleton.text {
height: 0.9em;
@media (prefers-reduced-motion: no-preference) {
.skeleton {
background-image: linear-gradient(
90deg,
var(--surface) 0%,
var(--surface-elevated) 50%,
var(--surface) 100%
);
background-size: 200% 100%;
background-repeat: no-repeat;
animation: skeleton-shimmer 1.2s ease-in-out infinite;
}
}
.radius-sm {

18
frontend/src/lib/components/Skeleton.svelte.test.ts
View File

@@ -46,4 +46,22 @@ describe('Skeleton primitive', () => {
render(Skeleton, { props: { testid: 'sk' } });
expect(screen.getByTestId('sk').classList.contains('skeleton')).toBe(true);
});
it('emits aspect-ratio inline (and no fixed height) so a fluid box keeps its shape', () => {
render(Skeleton, { props: { testid: 'sk', aspectRatio: '2 / 3' } });
const el = screen.getByTestId('sk') as HTMLElement;
expect(el.style.aspectRatio).toBe('2 / 3');
// A definite height would defeat aspect-ratio and collapse the box.
expect(el.style.height).toBe('');
});
it('gives a text-variant line a default height without a caller-supplied one', () => {
render(Skeleton, { props: { testid: 'sk', variant: 'text' } });
expect((screen.getByTestId('sk') as HTMLElement).style.height).toBe('0.9em');
});
it('lets an explicit height override the text-variant default', () => {
render(Skeleton, { props: { testid: 'sk', variant: 'text', height: '1rem' } });
expect((screen.getByTestId('sk') as HTMLElement).style.height).toBe('1rem');
});
});

14
frontend/src/lib/styles/tokens.css
View File

@@ -349,6 +349,20 @@ img {
gap: var(--space-4);
}
/* A single cover cell — the catalog/author/collection cards and the
loading skeleton share this so their layout can't drift. */
.manga-card {
display: flex;
flex-direction: column;
gap: var(--space-2);
list-style: none;
/* Grid items default to min-width: auto which equals the intrinsic
content size — long author / title strings then push the cell past
`1fr`. Forcing 0 lets the column control the width and the children
ellipsize inside it. */
min-width: 0;
}
@media (max-width: 640px) {
.manga-grid {
grid-template-columns: repeat(4, minmax(0, 1fr));

15
frontend/src/routes/+page.svelte
View File

@@ -660,7 +660,8 @@
</Sheet>
{#if loading}
<div data-testid="loading" role="status" aria-label="Loading manga">
<div data-testid="loading" role="status">
<span class="visually-hidden">Loading manga…</span>
<MangaGridSkeleton />
</div>
{:else if error}
@@ -975,6 +976,18 @@
color: var(--text-muted);
}
.visually-hidden {
position: absolute;
width: 1px;
height: 1px;
padding: 0;
margin: -1px;
overflow: hidden;
clip: rect(0, 0, 0, 0);
white-space: nowrap;
border: 0;
}
.error {
color: var(--danger);
}