refactor(person-mention): extract computeHoverCardPosition into testable util
Three reviewer concerns land here: - Felix #2: magic numbers 0.7 and 300 belong in named constants - Sara #6: the position function had 4 branches and 2 thresholds with zero tests - Leonie FINDING-05: at 320px viewport the flip-left could push the card past the right edge — needed a viewport clamp Move the function to src/lib/utils/hoverCardPosition.ts as a pure (rect, viewport) → {top, left} mapping, with named exports CARD_WIDTH_PX, CARD_HEIGHT_PX, CARD_GAP_PX, BOTTOM_BAND_RATIO, RIGHT_FLIP_THRESHOLD_PX. Add a viewport clamp so left + CARD_WIDTH never exceeds the right edge. Ten unit tests cover default placement, flip-up (both triggers), flip-left, flip-right-edge clamp, and scroll offset. TranscriptionReadView passes the current window viewport in on each call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
@@ -7,6 +7,7 @@ import {
|
||||
type SafeHtml,
|
||||
PERSON_MENTION_SELECTOR
|
||||
} from '$lib/utils/mention';
|
||||
import { computeHoverCardPosition } from '$lib/utils/hoverCardPosition';
|
||||
import PersonHoverCard from './PersonHoverCard.svelte';
|
||||
import type { HoverData, LoadState } from '$lib/types/personHoverCard';
|
||||
import { goto } from '$app/navigation';
|
||||
@@ -38,10 +39,6 @@ let activeCard: {
|
||||
position: { top: number; left: number };
|
||||
} | null = $state(null);
|
||||
|
||||
const CARD_WIDTH = 320;
|
||||
const CARD_HEIGHT = 180;
|
||||
const CARD_GAP = 6;
|
||||
|
||||
// Compose splitByMarkers with renderTranscriptionBody. Markers are pre-rendered
|
||||
// as <em data-marker> tags; text segments run through HTML-escaping + mention
|
||||
// substitution. The two are concatenated to preserve marker boundaries — markers
|
||||
@@ -81,27 +78,12 @@ function fetchHoverData(personId: string): Promise<HoverData | null> {
|
||||
return cached;
|
||||
}
|
||||
|
||||
function computeCardPosition(rect: DOMRect): { top: number; left: number } {
|
||||
const vw = window.innerWidth;
|
||||
const vh = window.innerHeight;
|
||||
|
||||
let top = rect.bottom + CARD_GAP;
|
||||
let left = rect.left;
|
||||
|
||||
// Flip up if the card would overflow the bottom edge OR the mention sits in
|
||||
// the bottom 30% of the viewport (Leonie #5329).
|
||||
if (vh - rect.bottom < CARD_HEIGHT + CARD_GAP || rect.top > vh * 0.7) {
|
||||
top = rect.top - CARD_HEIGHT - CARD_GAP;
|
||||
}
|
||||
|
||||
// Flip left if <300px from the right edge.
|
||||
if (vw - rect.left < 300) {
|
||||
left = rect.right - CARD_WIDTH;
|
||||
}
|
||||
|
||||
function currentViewport() {
|
||||
return {
|
||||
top: Math.max(0, top + window.scrollY),
|
||||
left: Math.max(0, left + window.scrollX)
|
||||
viewportWidth: window.innerWidth,
|
||||
viewportHeight: window.innerHeight,
|
||||
scrollX: window.scrollX,
|
||||
scrollY: window.scrollY
|
||||
};
|
||||
}
|
||||
|
||||
@@ -115,7 +97,7 @@ async function handleMentionEnter(event: Event) {
|
||||
link.setAttribute('aria-describedby', cardId);
|
||||
|
||||
const rect = link.getBoundingClientRect();
|
||||
const position = computeCardPosition(rect);
|
||||
const position = computeHoverCardPosition(rect, currentViewport());
|
||||
|
||||
activeCard = { personId, cardId, position, state: { status: 'loading' } };
|
||||
|
||||
|
||||
152
frontend/src/lib/utils/hoverCardPosition.spec.ts
Normal file
152
frontend/src/lib/utils/hoverCardPosition.spec.ts
Normal file
@@ -0,0 +1,152 @@
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import {
|
||||
computeHoverCardPosition,
|
||||
CARD_WIDTH_PX,
|
||||
CARD_HEIGHT_PX,
|
||||
CARD_GAP_PX,
|
||||
BOTTOM_BAND_RATIO,
|
||||
RIGHT_FLIP_THRESHOLD_PX
|
||||
} from './hoverCardPosition';
|
||||
|
||||
const makeRect = (overrides: Partial<DOMRect> = {}): DOMRect => {
|
||||
const base = { top: 100, left: 200, bottom: 120, right: 300, width: 100, height: 20 };
|
||||
const merged = { ...base, ...overrides };
|
||||
return {
|
||||
...merged,
|
||||
x: merged.left,
|
||||
y: merged.top,
|
||||
toJSON: () => merged
|
||||
} as DOMRect;
|
||||
};
|
||||
|
||||
describe('computeHoverCardPosition', () => {
|
||||
it('exports the spec constants used by the spec/CSS layer', () => {
|
||||
// Pin the values the design spec calls out — if these drift, the design spec
|
||||
// in #5329 needs to drift with them. Felix's PR review #2 (named constants).
|
||||
expect(CARD_WIDTH_PX).toBe(320);
|
||||
expect(CARD_HEIGHT_PX).toBe(180);
|
||||
expect(CARD_GAP_PX).toBe(6);
|
||||
expect(BOTTOM_BAND_RATIO).toBe(0.7);
|
||||
expect(RIGHT_FLIP_THRESHOLD_PX).toBe(300);
|
||||
});
|
||||
|
||||
describe('default placement (below-right)', () => {
|
||||
it('positions the card below the rect with a small gap', () => {
|
||||
const rect = makeRect({ top: 100, bottom: 120, left: 200 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.top).toBe(120 + CARD_GAP_PX);
|
||||
expect(result.left).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
describe('flip-up rule (Leonie #5329)', () => {
|
||||
it('flips up when the card would overflow the bottom edge', () => {
|
||||
// Mention sits 50px above the viewport bottom — card is 180px tall, can't fit below
|
||||
const rect = makeRect({ top: 800, bottom: 850 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.top).toBe(800 - CARD_HEIGHT_PX - CARD_GAP_PX);
|
||||
});
|
||||
|
||||
it('flips up when the mention sits in the bottom 30% of the viewport (BOTTOM_BAND_RATIO)', () => {
|
||||
// rect.top is at 80% of viewport — fits below numerically, but poor UX
|
||||
const rect = makeRect({ top: 720, bottom: 740 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.top).toBe(720 - CARD_HEIGHT_PX - CARD_GAP_PX);
|
||||
});
|
||||
});
|
||||
|
||||
describe('flip-left rule', () => {
|
||||
it('flips left when the rect is within RIGHT_FLIP_THRESHOLD_PX of the right edge', () => {
|
||||
// vw - rect.left = 1440 - 1200 = 240 < 300, so flip
|
||||
const rect = makeRect({ left: 1200, right: 1300, top: 100, bottom: 120 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
// left = right - CARD_WIDTH = 1300 - 320 = 980
|
||||
expect(result.left).toBe(980);
|
||||
});
|
||||
|
||||
it('does not flip left when the rect has plenty of right-side room', () => {
|
||||
// vw - rect.left = 1440 - 200 = 1240 >> 300 → no flip
|
||||
const rect = makeRect({ left: 200, right: 300 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.left).toBe(200);
|
||||
});
|
||||
});
|
||||
|
||||
describe('viewport clamping (Leonie FINDING-05)', () => {
|
||||
it('clamps left so the card never overflows the right edge', () => {
|
||||
// On a 320px viewport, even with flip the card width equals the viewport.
|
||||
// Without clamping the card would be at left=0 but extend to 320 — fine.
|
||||
// At viewport=400px with rect.left=200, flip puts left=300-320=-20, clamped to 0.
|
||||
const rect = makeRect({ left: 200, right: 300, top: 100, bottom: 120 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 400,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.left).toBeGreaterThanOrEqual(0);
|
||||
expect(result.left + CARD_WIDTH_PX).toBeLessThanOrEqual(400);
|
||||
});
|
||||
|
||||
it('never returns a negative top or left', () => {
|
||||
const rect = makeRect({ top: -50, left: -100, bottom: -30, right: 0 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.top).toBeGreaterThanOrEqual(0);
|
||||
expect(result.left).toBeGreaterThanOrEqual(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('scroll offset', () => {
|
||||
it('adds window.scrollY to the absolute-positioned top', () => {
|
||||
const rect = makeRect({ top: 100, bottom: 120 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 0,
|
||||
scrollY: 500
|
||||
});
|
||||
expect(result.top).toBe(120 + CARD_GAP_PX + 500);
|
||||
});
|
||||
|
||||
it('adds window.scrollX to the absolute-positioned left', () => {
|
||||
const rect = makeRect({ top: 100, bottom: 120, left: 200, right: 300 });
|
||||
const result = computeHoverCardPosition(rect, {
|
||||
viewportWidth: 1440,
|
||||
viewportHeight: 900,
|
||||
scrollX: 50,
|
||||
scrollY: 0
|
||||
});
|
||||
expect(result.left).toBe(200 + 50);
|
||||
});
|
||||
});
|
||||
});
|
||||
69
frontend/src/lib/utils/hoverCardPosition.ts
Normal file
69
frontend/src/lib/utils/hoverCardPosition.ts
Normal file
@@ -0,0 +1,69 @@
|
||||
/**
|
||||
* Pure positioning logic for the person-mention hover card.
|
||||
*
|
||||
* Pulled out of TranscriptionReadView so the four placement branches
|
||||
* (default, flip-up, flip-left, both) plus the viewport clamp are unit-testable
|
||||
* without DOM. Sara's PR-B2 review #6 (no test for computeCardPosition) and
|
||||
* Leonie's FINDING-05 (320px overflow) both land here.
|
||||
*/
|
||||
|
||||
/** Width of the rendered hover card. Mirrored in PersonHoverCard.svelte's CSS. */
|
||||
export const CARD_WIDTH_PX = 320;
|
||||
|
||||
/** Min-height of the rendered hover card. Mirrored in PersonHoverCard.svelte's CSS. */
|
||||
export const CARD_HEIGHT_PX = 180;
|
||||
|
||||
/** Gap between the mention rect and the card so they do not touch. */
|
||||
export const CARD_GAP_PX = 6;
|
||||
|
||||
/**
|
||||
* Mentions in the bottom 30% of the viewport flip the card up by default,
|
||||
* even if it would numerically fit below — keeping the eye-line stable
|
||||
* is more important than minimal travel (Leonie #5329).
|
||||
*/
|
||||
export const BOTTOM_BAND_RATIO = 0.7;
|
||||
|
||||
/**
|
||||
* Mentions within this distance of the right viewport edge flip the card
|
||||
* left so it stays fully visible.
|
||||
*/
|
||||
export const RIGHT_FLIP_THRESHOLD_PX = 300;
|
||||
|
||||
export type Viewport = {
|
||||
viewportWidth: number;
|
||||
viewportHeight: number;
|
||||
scrollX: number;
|
||||
scrollY: number;
|
||||
};
|
||||
|
||||
export type CardPosition = { top: number; left: number };
|
||||
|
||||
/**
|
||||
* Compute absolute-positioned top/left for the hover card, given a rect for
|
||||
* the mention anchor and the current viewport. Output is in document
|
||||
* coordinates (already includes scroll offsets).
|
||||
*/
|
||||
export function computeHoverCardPosition(rect: DOMRect, vp: Viewport): CardPosition {
|
||||
let top = rect.bottom + CARD_GAP_PX;
|
||||
let left = rect.left;
|
||||
|
||||
const overflowsBottom = vp.viewportHeight - rect.bottom < CARD_HEIGHT_PX + CARD_GAP_PX;
|
||||
const inBottomBand = rect.top > vp.viewportHeight * BOTTOM_BAND_RATIO;
|
||||
if (overflowsBottom || inBottomBand) {
|
||||
top = rect.top - CARD_HEIGHT_PX - CARD_GAP_PX;
|
||||
}
|
||||
|
||||
if (vp.viewportWidth - rect.left < RIGHT_FLIP_THRESHOLD_PX) {
|
||||
left = rect.right - CARD_WIDTH_PX;
|
||||
}
|
||||
|
||||
// Clamp left so the card never extends past the right viewport edge
|
||||
// (FINDING-05: at 320px viewport the flip would otherwise produce a
|
||||
// negative left or right-side overflow).
|
||||
left = Math.min(left, vp.viewportWidth - CARD_WIDTH_PX - CARD_GAP_PX);
|
||||
|
||||
return {
|
||||
top: Math.max(0, top + vp.scrollY),
|
||||
left: Math.max(0, left + vp.scrollX)
|
||||
};
|
||||
}
|
||||
Reference in New Issue
Block a user