From bf81b8d3df512239eff24af905007ce25568580e Mon Sep 17 00:00:00 2001 From: Andras Schmelczer Date: Wed, 3 Jun 2026 20:05:16 +0100 Subject: [PATCH] Review feedback --- Dockerfile | 15 ++++++- docker-compose.dev.yml | 8 +++- frontend/e2e/smoke.spec.ts | 35 +++++++++++++++ frontend/safety-worker.js | 38 ++++++++++++++++ .../components/modal/block-edit.component.ts | 40 ++++++++++++++--- .../app/components/modal/modal.component.ts | 10 +++++ .../app/components/page/page.component.html | 1 + .../app/components/page/page.component.scss | 45 +++++++++++++------ .../src/app/components/page/page.component.ts | 32 ++++++++++++- .../app/components/pages/pages.component.scss | 9 ++-- .../double-slider/double-slider.component.ts | 5 ++- frontend/src/app/services/store.service.ts | 3 +- frontend/src/index.html | 5 ++- 13 files changed, 218 insertions(+), 28 deletions(-) create mode 100644 frontend/safety-worker.js diff --git a/Dockerfile b/Dockerfile index 6327bb9..483f8e6 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,11 +7,24 @@ WORKDIR /build # this. The trailing slash matters — it becomes and the service # worker (ngsw.json) URL prefix. ARG BASE_HREF=/ +# Toggle the built-in PWA service worker. Defaults to "enabled" so prod images +# ship the full offline-capable PWA unchanged. The dev / e2e compose overrides +# this to "disabled" (see docker-compose.dev.yml): it swaps Angular's real +# ngsw-worker.js for a self-unregistering safety worker and drops ngsw.json, so +# a `--build` rebuild is never shadowed by a stale, client-cached app shell on +# the fixed localhost origin. (Docker layer caching already busts the SPA build +# on FE source changes; this handles the browser-side service-worker cache.) +ARG SERVICE_WORKER=enabled COPY frontend/package.json frontend/package-lock.json ./ RUN npm ci COPY frontend/ ./ -RUN npm run build -- --base-href="$BASE_HREF" # Angular's application builder outputs to dist/frontend/browser/ +RUN npm run build -- --base-href="$BASE_HREF" \ + && if [ "$SERVICE_WORKER" = "disabled" ]; then \ + echo "Neutralising service worker for dev/e2e image"; \ + rm -f dist/frontend/browser/ngsw.json; \ + cp safety-worker.js dist/frontend/browser/ngsw-worker.js; \ + fi # Stage 2: runtime FROM python:3.13-slim diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index f9f3fd3..c6db81e 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -9,7 +9,13 @@ services: life-towers: - build: . + build: + context: . + args: + # Strip the PWA service worker from this image so `up --build` always + # serves the freshly-built FE assets instead of a stale, SW-cached app + # shell. See the SERVICE_WORKER arg in the Dockerfile. + SERVICE_WORKER: disabled image: life-towers:dev ports: - "8000:8000" diff --git a/frontend/e2e/smoke.spec.ts b/frontend/e2e/smoke.spec.ts index 2493014..1478652 100644 --- a/frontend/e2e/smoke.spec.ts +++ b/frontend/e2e/smoke.spec.ts @@ -180,4 +180,39 @@ test.describe('Life Towers smoke test', () => { }); await expectTaskListOpen(page, 'Clean up alerts'); }); + + // Regression: opening the block-edit carousel must position the target card + // INSTANTLY, not animate a scroll across the whole strip. The carousel sets + // `scroll-behavior: smooth`, so scrollTo({behavior:'auto'}) would animate — + // very visible on mobile where the strip can be thousands of px wide. + test('block-edit carousel opens centered without animating the scroll', async ({ page }) => { + await page.setViewportSize({ width: 390, height: 844 }); + await page.goto('/'); + + await expect(page.getByText('Welcome to Life Towers')).toBeVisible({ timeout: 15000 }); + await page.getByRole('button', { name: 'Load sample towers' }).click(); + await page.waitForSelector('section.modal', { state: 'detached' }); + // Let the falling animation settle. + await page.waitForTimeout(1800); + + // Open the carousel on a done block deep in the strip (the last square). + const squares = page.locator('lt-block'); + await squares.nth(await squares.count() - 1).click(); + await page.waitForSelector('lt-block-edit .carousel'); + + // Sample scrollLeft immediately and a frame later: an animated scroll would + // still be moving; an instant jump is already at its final value. + const carousel = page.locator('lt-block-edit .carousel'); + const first = await carousel.evaluate((c: HTMLElement) => c.scrollLeft); + await page.waitForTimeout(60); + const second = await carousel.evaluate((c: HTMLElement) => c.scrollLeft); + expect(Math.abs(second - first)).toBeLessThan(2); // not mid-animation + + // The active card is centered: left/right viewport gaps match within a few px. + const gaps = await page.locator('lt-block-edit .card.active').evaluate((el: HTMLElement) => { + const r = el.getBoundingClientRect(); + return { left: r.left, right: window.innerWidth - r.right }; + }); + expect(Math.abs(gaps.left - gaps.right)).toBeLessThan(8); + }); }); diff --git a/frontend/safety-worker.js b/frontend/safety-worker.js new file mode 100644 index 0000000..e5f867a --- /dev/null +++ b/frontend/safety-worker.js @@ -0,0 +1,38 @@ +/* + * Safety service worker — shipped ONLY in the dev / e2e Docker image + * (Dockerfile build arg SERVICE_WORKER=disabled) in place of Angular's real + * ngsw-worker.js. + * + * Why it exists: the dev compose builds a production SPA bundle, so the PWA + * service worker is enabled (`enabled: !isDevMode()` in app.config.ts). On the + * fixed localhost:8000 origin it would keep serving a stale, client-cached app + * shell across `docker compose -f docker-compose.dev.yml up --build` rebuilds, + * shadowing freshly-built FE assets. + * + * This worker's only job is to disappear: it claims any open clients, deletes + * every Cache Storage entry, then unregisters itself. Any previously-installed + * real service worker is evicted on its next update check (the browser fetches + * this changed ngsw-worker.js and replaces the old one with this), and no + * service worker is ever left controlling the page — so assets are always + * fetched from network, where index.html is `no-cache` and bundles are + * content-hashed. + * + * It deliberately does NOT call client.navigate(): the bundle re-registers on + * every load, so a forced reload would create a loop. The only cost is a + * harmless register -> unregister cycle per load. + */ +self.addEventListener('install', () => self.skipWaiting()); + +self.addEventListener('activate', (event) => { + event.waitUntil( + (async () => { + try { + await self.clients.claim(); + const keys = await caches.keys(); + await Promise.all(keys.map((key) => caches.delete(key))); + } finally { + await self.registration.unregister(); + } + })(), + ); +}); diff --git a/frontend/src/app/components/modal/block-edit.component.ts b/frontend/src/app/components/modal/block-edit.component.ts index 58cad90..60223fc 100644 --- a/frontend/src/app/components/modal/block-edit.component.ts +++ b/frontend/src/app/components/modal/block-edit.component.ts @@ -68,8 +68,8 @@ export function createDoneValue(defaultDone: boolean, currentDone: boolean, edit tabindex="0" [attr.aria-label]="'Focus ' + (editedFor(b.id).tag || 'block')" (click)="onCardClick(i + 1)" - (keydown.enter)="onCardClick(i + 1)" - (keydown.space)="$event.preventDefault(); onCardClick(i + 1)" + (keydown.enter)="onCardKey($event, i + 1)" + (keydown.space)="onCardKey($event, i + 1)" >
@@ -151,8 +151,8 @@ export function createDoneValue(defaultDone: boolean, currentDone: boolean, edit tabindex="0" aria-label="Focus create card" (click)="onCardClick(blocks().length + 1)" - (keydown.enter)="onCardClick(blocks().length + 1)" - (keydown.space)="$event.preventDefault(); onCardClick(blocks().length + 1)" + (keydown.enter)="onCardKey($event, blocks().length + 1)" + (keydown.space)="onCardKey($event, blocks().length + 1)" >
@@ -242,6 +242,12 @@ export function createDoneValue(defaultDone: boolean, currentDone: boolean, edit left: 0; right: 0; bottom: 0; + // Cover the *visible* viewport. Sizing via top:0/bottom:0 uses the layout + // viewport, which on mobile extends behind the browser's bottom toolbar + // (and the soft keyboard) — so the vertically-centered card gets its + // bottom cut off. 100dvh tracks the area that's actually on screen; + // browsers without dvh fall back to the top/bottom inset above. + height: 100dvh; z-index: 10001; // above modal backdrop (10000) @media (max-height: $min-height) { @@ -279,6 +285,12 @@ export function createDoneValue(defaultDone: boolean, currentDone: boolean, edit @media (max-width: $mobile-width) { padding: var(--title-clearance) var(--medium-padding) var(--medium-padding); + // Keep the card centered when it fits, but never clip it: 'safe center' + // falls back to top-alignment when the card is taller than the visible + // viewport, and overflow-y lets the user scroll down to the bottom + // (delete/create button) — e.g. when the soft keyboard shrinks the view. + align-items: safe center; + overflow-y: auto; } @media (max-height: $min-height) { @@ -788,6 +800,19 @@ export class BlockEditComponent implements AfterViewInit { } } + /** + * Activate a card via Space/Enter only when the card itself is focused. The + * card is a role="button" that wraps the description textarea and the tag + * input; without this guard the keydown bubbles up from those fields and the + * space handler's preventDefault() swallows the space, making it impossible + * to type spaces while editing a block. + */ + onCardKey(event: Event, idx: number): void { + if (event.target !== event.currentTarget) return; + event.preventDefault(); + this.onCardClick(idx); + } + /** Close the carousel when the user clicks anywhere that isn't a real card. */ onBackdropClick(event: MouseEvent): void { const target = event.target as HTMLElement | null; @@ -815,7 +840,12 @@ export class BlockEditComponent implements AfterViewInit { if (!card) return; const left = card.offsetLeft - (container.clientWidth - card.offsetWidth) / 2; - container.scrollTo({ left, behavior: smooth ? 'smooth' : 'auto' }); + // 'instant' (not 'auto') is required: the carousel sets `scroll-behavior: + // smooth`, and 'auto' defers to that — so the initial open would *animate* + // a scroll across the whole strip to reach the target card (very visible on + // mobile, where the carousel can be thousands of px wide). Tap-to-navigate + // still passes smooth=true for the nice slide between cards. + container.scrollTo({ left, behavior: smooth ? 'smooth' : 'instant' }); this.activeIdx.set(idx); } diff --git a/frontend/src/app/components/modal/modal.component.ts b/frontend/src/app/components/modal/modal.component.ts index 47c9eb8..097e12e 100644 --- a/frontend/src/app/components/modal/modal.component.ts +++ b/frontend/src/app/components/modal/modal.component.ts @@ -97,11 +97,21 @@ export class ModalComponent implements AfterViewInit, OnDestroy { private readonly dialogRef = viewChild>('dialog'); private previousFocus: HTMLElement | null = null; private readonly modalState = inject(ModalStateService); + private readonly host = inject>(ElementRef); ngAfterViewInit(): void { this.previousFocus = document.activeElement as HTMLElement; // Track open state so towers can be locked while any modal is mounted. this.modalState.open(); + // Hoist the modal to so its position:fixed references the viewport. + // Tower-level modals (block-edit, tower-settings) are rendered inside the + // .towers horizontal scroll container; on iOS Safari a position:fixed + // descendant of a scrolling/overflow ancestor is clipped to that ancestor's + // box instead of the viewport — cutting off the card's bottom and confining + // the backdrop to the towers band (title + sliders show through). Moving the + // host out of that ancestor restores true viewport-fixed behaviour. Angular + // removes the node via its *current* parent on destroy, so this is safe. + document.body.appendChild(this.host.nativeElement); // Defer one tick so the opacity transition runs (0 → 1). setTimeout(() => this.active.set(true), 0); } diff --git a/frontend/src/app/components/page/page.component.html b/frontend/src/app/components/page/page.component.html index 431d960..0b87386 100644 --- a/frontend/src/app/components/page/page.component.html +++ b/frontend/src/app/components/page/page.component.html @@ -7,6 +7,7 @@ @for (tower of page().towers; track tower.id) { * { + &>* { width: 100%; max-width: 200px; box-sizing: border-box; @@ -69,13 +71,12 @@ // Mobile: fixed-width towers with horizontal scroll (1.5-column rhythm). @media (max-width: $mobile-width) { --mobile-tower-width: calc(66vw - var(--small-padding)); - --mobile-tower-side-padding: max( - var(--medium-padding), - calc((100% - var(--mobile-tower-width)) / 2) - ); + --mobile-tower-side-padding: max(var(--medium-padding), + calc((100% - var(--mobile-tower-width)) / 2)); overflow-x: auto; - overflow-y: visible; + overflow-y: hidden; + touch-action: pan-x; -webkit-overflow-scrolling: touch; scroll-snap-type: x mandatory; scroll-padding-inline: var(--mobile-tower-side-padding); @@ -89,7 +90,7 @@ display: none; } - & > * { + &>* { width: var(--mobile-tower-width) !important; max-width: var(--mobile-tower-width) !important; min-width: var(--mobile-tower-width) !important; @@ -102,19 +103,28 @@ .double-slider-container { width: 100%; transition: opacity $long-animation-time; - @media (max-height: $min-height) { display: none; } - &.transparent { opacity: 0; pointer-events: none; } + + @media (max-height: $min-height) { + display: none; + } + + &.transparent { + opacity: 0; + pointer-events: none; + } } .confirm-delete { @include card(); width: 66vw; max-width: 500px; + @media (max-width: $mobile-width) { width: 88vw; max-width: 88vw; padding: var(--medium-padding); } + box-sizing: border-box; padding: var(--large-padding); position: relative; @@ -124,6 +134,7 @@ .header { @include center-child(); + .exit { position: absolute; right: var(--large-padding); @@ -132,7 +143,9 @@ } p { - strong { font-weight: bold; } + strong { + font-weight: bold; + } } .confirm-buttons { @@ -147,13 +160,19 @@ button.danger { color: #b53f3f; border-bottom-color: #b53f3f55; - &:after { background-color: #b53f3f; } + + &:after { + background-color: #b53f3f; + } } @media (max-width: $mobile-width) { flex-direction: column; gap: var(--small-padding); - button { width: 100%; } + + button { + width: 100%; + } } } } @@ -180,4 +199,4 @@ transform: translateX(-50%) scale(1.1); } } -} +} \ No newline at end of file diff --git a/frontend/src/app/components/page/page.component.ts b/frontend/src/app/components/page/page.component.ts index 544ed13..3e5a054 100644 --- a/frontend/src/app/components/page/page.component.ts +++ b/frontend/src/app/components/page/page.component.ts @@ -9,6 +9,9 @@ import { HostListener, effect, untracked, + ElementRef, + Injector, + afterNextRender, } from '@angular/core'; import { Page } from '../../models'; import { StoreService } from '../../services/store.service'; @@ -69,6 +72,8 @@ export class PageComponent { protected readonly store = inject(StoreService); private readonly modalState = inject(ModalStateService); + private readonly host = inject>(ElementRef); + private readonly injector = inject(Injector); /** True while any lt-modal is mounted — used to lock tower drag. */ readonly modalOpen = this.modalState.anyOpen; readonly mobileDragDisabled = signal(this.isMobileViewport()); @@ -146,7 +151,32 @@ export class PageComponent { onAddTower(result: TowerSettingsResult): void { this.showAddTower.set(false); - this.store.addTower(this.page().id, result.name, result.base_color); + const towerId = this.store.addTower(this.page().id, result.name, result.base_color); + this.centerTowerOnMobile(towerId); + } + + /** + * On mobile the tower row is a horizontal scroll-snap container. Adding a + * tower appends it next to the (far-right) "+" button, so without this the + * view stays scrolled on the "+" button rather than the new tower. Center + * the freshly-created tower once it's painted. No-op on desktop, where the + * row is centered and doesn't scroll. + */ + private centerTowerOnMobile(towerId: string): void { + if (!this.isMobileViewport()) return; + afterNextRender( + () => { + const container = this.host.nativeElement.querySelector('.towers'); + const tower = container?.querySelector(`[data-tower-id="${towerId}"]`); + if (!container || !tower) return; + const containerRect = container.getBoundingClientRect(); + const towerRect = tower.getBoundingClientRect(); + const delta = + towerRect.left + towerRect.width / 2 - (containerRect.left + containerRect.width / 2); + container.scrollTo({ left: container.scrollLeft + delta, behavior: 'smooth' }); + }, + { injector: this.injector }, + ); } onUpdateTower(towerId: string, result: TowerSettingsResult): void { diff --git a/frontend/src/app/components/pages/pages.component.scss b/frontend/src/app/components/pages/pages.component.scss index cdb7480..3413dbb 100644 --- a/frontend/src/app/components/pages/pages.component.scss +++ b/frontend/src/app/components/pages/pages.component.scss @@ -15,6 +15,7 @@ margin: 0 auto; position: relative; z-index: 1000; + @media (max-width: $mobile-width) { width: 80vw; max-width: 320px; @@ -27,8 +28,9 @@ // Generous breathing room between the page selector dropdown and the // towers — the dropdown can open downward without crowding the towers. padding-top: var(--large-padding); + @media (max-width: $mobile-width) { - padding-top: var(--medium-padding); + padding-top: var(--small-padding); } } @@ -40,7 +42,6 @@ } @media (max-width: $mobile-width) { - margin-top: var(--medium-padding); font-size: var(--medium-font-size); } } @@ -49,11 +50,13 @@ @include card(); width: 66vw; max-width: 500px; + @media (max-width: $mobile-width) { width: 88vw; max-width: 88vw; padding: var(--medium-padding); } + box-sizing: border-box; padding: var(--large-padding); position: relative; @@ -99,4 +102,4 @@ } } } -} +} \ No newline at end of file diff --git a/frontend/src/app/components/shared/double-slider/double-slider.component.ts b/frontend/src/app/components/shared/double-slider/double-slider.component.ts index f4db1e4..0ee4469 100644 --- a/frontend/src/app/components/shared/double-slider/double-slider.component.ts +++ b/frontend/src/app/components/shared/double-slider/double-slider.component.ts @@ -68,6 +68,7 @@ export interface DoubleSliderRange { @media (max-width: $mobile-width) { max-width: 90vw; margin-top: calc(#{$slider-size} / 2); + height: 54px; } label { display: none; } @@ -153,8 +154,8 @@ export interface DoubleSliderRange { @media (max-width: $mobile-width) { font-size: var(--small-font-size); - margin-top: $slider-size; - span { margin-top: 10px; } + margin-top: calc(#{$slider-size} - 12px); + span { margin-top: 8px; } } } } diff --git a/frontend/src/app/services/store.service.ts b/frontend/src/app/services/store.service.ts index 71589bc..2431c91 100644 --- a/frontend/src/app/services/store.service.ts +++ b/frontend/src/app/services/store.service.ts @@ -311,7 +311,7 @@ export class StoreService implements OnDestroy { if (changed) this.scheduleSave(); } - addTower(pageId: string, name: string, base_color: HslColor): void { + addTower(pageId: string, name: string, base_color: HslColor): string { const tower: Tower = { id: uuidV4(), name, base_color, blocks: [] }; this._pages.update((pages) => pages.map((p) => (p.id === pageId ? { ...p, towers: [...p.towers, tower] } : p)), @@ -319,6 +319,7 @@ export class StoreService implements OnDestroy { this.analytics.trackStart(); this.analytics.trackTowerCreated(); this.scheduleSave(); + return tower.id; } updateTower(pageId: string, towerId: string, patch: Partial>): void { diff --git a/frontend/src/index.html b/frontend/src/index.html index 147335a..7ce5d22 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -4,7 +4,10 @@ Life Towers - +