Review feedback

This commit is contained in:
Andras Schmelczer 2026-06-03 20:05:16 +01:00
parent e00300de6c
commit bf81b8d3df
13 changed files with 218 additions and 28 deletions

View file

@ -7,11 +7,24 @@ WORKDIR /build
# this. The trailing slash matters — it becomes <base href> 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

View file

@ -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"

View file

@ -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);
});
});

38
frontend/safety-worker.js Normal file
View file

@ -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();
}
})(),
);
});

View file

@ -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)"
>
<div class="mask"></div>
@ -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)"
>
<div class="mask"></div>
@ -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);
}

View file

@ -97,11 +97,21 @@ export class ModalComponent implements AfterViewInit, OnDestroy {
private readonly dialogRef = viewChild<ElementRef<HTMLElement>>('dialog');
private previousFocus: HTMLElement | null = null;
private readonly modalState = inject(ModalStateService);
private readonly host = inject<ElementRef<HTMLElement>>(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 <body> 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);
}

View file

@ -7,6 +7,7 @@
@for (tower of page().towers; track tower.id) {
<lt-tower
cdkDrag
[attr.data-tower-id]="tower.id"
[cdkDragDisabled]="modalOpen() || mobileDragDisabled()"
[tower]="tower"
[dateRange]="dateRange()"

View file

@ -39,8 +39,10 @@
.add-tower-wrapper {
@include center-child();
img.add-tower {
height: 48px;
@media (max-width: $mobile-width) {
height: 32px;
}
@ -55,7 +57,7 @@
}
}
& > * {
&>* {
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);
}
}
}
}

View file

@ -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<HTMLElement>>(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<HTMLElement>('.towers');
const tower = container?.querySelector<HTMLElement>(`[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 {

View file

@ -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 @@
}
}
}
}
}

View file

@ -68,6 +68,7 @@ export interface DoubleSliderRange<T> {
@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<T> {
@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; }
}
}
}

View file

@ -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<Omit<Tower, 'id' | 'blocks'>>): void {

View file

@ -4,7 +4,10 @@
<meta charset="utf-8" />
<title>Life Towers</title>
<base href="/" />
<meta name="viewport" content="width=device-width, initial-scale=1.0" />
<meta
name="viewport"
content="width=device-width, initial-scale=1.0, maximum-scale=1.0, user-scalable=no"
/>
<meta name="theme-color" content="#5d576b" />
<meta name="description" content="Organise your tasks into visual towers of blocks, grouped on pages." />