From e36bd85f54785ea46a8203b8b7f131570a91a5c4 Mon Sep 17 00:00:00 2001 From: Roland Date: Sun, 2 Aug 2026 11:35:49 -0600 Subject: [PATCH] ui: act on review -- corner chrome, a swallowed className, and the poster frame Three real defects from the PR review, each reproduced before it was fixed. The tag mark and a video's duration were pinned to the poster BAND, which is the full width of the card, rather than to the painted image. Once the height cap landed the two stopped being the same box: measured, the mark sat 39px clear of a book cover, 56px clear of a small poster and 60px clear of a vertical video, and the duration 55px clear. It went unnoticed because on a 16:9 poster the image fills the band and the two pin to the same place -- so the tests cover the height-bound ratios, and assert the letterbox gap is really there before measuring into it. CardPoster now centres a frame that shrink-wraps the image, and the frame is what corner chrome is positioned against. VideoCard passes its duration through a new `overlay` prop rather than wrapping CardPoster in a box of its own. The mark also went back INSIDE the link. Splitting it out made the tagged corner a dead spot on an otherwise clickable poster. CardLink and ExternalCardLink wrote `className='...' {...props}`, so a caller's className replaced the pair instead of adding to it. Every archive title passes `card-title-ellipsis`, so every archive title lost `card-link` -- the one card type this PR migrates for layout was the one missing the title treatment. Both compose now. On the fourth comment, the breadth of the ActionIcon remap: it is deliberate and the numbers are small. 59 IconButton call sites, 52 of them at the default size, 7 at `xs` (one in Flasher, three in the inventory schema editor). The one library control that stands beside an input rather than a button is SearchBox's clear, and the remap is exactly what that scale is for -- it now matches its field to the pixel, where before it was 28px against a 36px input. Guarded by a test that fails at 42 vs 36 if the default is reverted. 1078 jest, 158 in ui-layout.cy.js, clean build. Co-Authored-By: Claude Opus 5 (1M context) --- app/src/components/Common.js | 44 ++++++++--- app/src/components/Common.test.js | 25 +++++- app/src/components/Videos.js | 12 ++- app/src/components/ui/ui-layout.cy.js | 106 +++++++++++++++++++++++++- app/src/components/ui/ui.css | 20 ++++- 5 files changed, 190 insertions(+), 17 deletions(-) diff --git a/app/src/components/Common.js b/app/src/components/Common.js index 566a6de2..217a2235 100644 --- a/app/src/components/Common.js +++ b/app/src/components/Common.js @@ -174,19 +174,30 @@ export function isoDatetimeToString(dt, time = false) { return d; } -export function CardLink({to, newTab = false, ...props}) { +/* + * Both card links COMPOSE the caller's className rather than being overwritten by it. + * Written as `className='...' {...props}`, the spread put the caller's class last and it + * replaced the pair outright -- so every archive title, which passes + * `card-title-ellipsis`, lost `card-link` and rendered in the link colour instead of the + * card's text colour. The one card type whose classes are set this way, and the one whose + * title looked wrong. + */ +const cardLinkClass = (className) => + ['no-link-underscore', 'card-link', className].filter(Boolean).join(' '); + +export function CardLink({to, newTab = false, className, ...props}) { props = newTab === true ? {...props, target: '_blank', rel: 'noopener noreferrer'} : props; - return + return {props.children} } -export function ExternalCardLink({to, children, ...props}) { +export function ExternalCardLink({to, children, className, ...props}) { return {children} @@ -726,7 +737,7 @@ export function findPosterPath(file) { } } -export function CardPoster({to, file}) { +export function CardPoster({to, file, overlay}) { const navigate = useNavigate(); // Marks a tagged file, pinned to the poster's corner. @@ -739,16 +750,29 @@ export function CardPoster({to, file}) { // FileGroup has a poster (screenshot/thumbnail) file. posterPath = `/media/${encodeMediaPath(posterPath)}`; - // Sized by `.wrolpi-card-poster`: as wide as the card allows, natural ratio. - const image = poster; + /* + * Sized by `.wrolpi-card-poster`: as wide as the card allows, capped in height, + * natural ratio. The corner chrome goes INSIDE the link, beside the image: + * + * - the frame shrink-wraps the painted image, so a mark pinned to its corner + * tracks the poster rather than the letterbox space beside it. A height-capped + * poster is a good deal narrower than its band -- 155px inside a 233px card for + * a book cover, less for a vertical video; + * - and the mark stays part of the poster's hit target, rather than a dead spot + * on the corner of an otherwise clickable image. + */ + const inner = <> + {imageLabel} + poster + {overlay} + ; return
- {imageLabel} {to // Link within this App. - ? {image} + ? {inner} // Preview the file. - : {image}} + : {inner}}
} else { // FileGroup has no poster. diff --git a/app/src/components/Common.test.js b/app/src/components/Common.test.js index e606e704..e455bf03 100644 --- a/app/src/components/Common.test.js +++ b/app/src/components/Common.test.js @@ -2,7 +2,8 @@ import React from 'react'; import {act, render, screen, waitFor} from '../test-utils'; import userEvent from '@testing-library/user-event'; import { - contrastingColor, contrastRatio, DirectorySearch, HelpHeader, InfoHeader, loadRole, + CardLink, contrastingColor, contrastRatio, DirectorySearch, ExternalCardLink, HelpHeader, + InfoHeader, loadRole, SearchResultsInput, TAG_TEXT_DARK, TAG_TEXT_LIGHT, } from './Common'; import {healthRole} from './admin/ControllerPage'; @@ -684,3 +685,25 @@ describe('a heading keeps its help icon on the same row as the text', () => { expect(container.querySelector('.inline-header')).toBeNull(); }); }); + +describe('card links', () => { + /* + * `.card-link` is what makes a card's title read as the card's text rather than as a + * link, so a title that misses it is the one thing on the card in the wrong colour. + * Both helpers set the class and then spread the caller's props over it, so any call + * site passing a className of its own -- which is every archive title, via + * `card-title-ellipsis` -- silently replaced the class instead of adding to it. + */ + it.each([ + ['CardLink', Title], + ['ExternalCardLink', + Title], + ])('%s keeps its own classes when the call site adds one', (_name, element) => { + render(element); + + const link = screen.getByText('Title'); + expect(link).toHaveClass('card-link'); + expect(link).toHaveClass('no-link-underscore'); + expect(link).toHaveClass('card-title-ellipsis'); + }); +}); diff --git a/app/src/components/Videos.js b/app/src/components/Videos.js index 31fb9381..eb22fafd 100644 --- a/app/src/components/Videos.js +++ b/app/src/components/Videos.js @@ -841,10 +841,14 @@ export function VideoCard({file}) { channel_url = `/videos/channel/${channel.id}/video`; } - const media =
- - -
; + /* + * The duration goes through CardPoster's `overlay` rather than being pinned to a + * wrapper around it: the wrapper is the full width of the card, and a vertical video's + * poster is only about half that once the height cap applies, so the badge landed well + * clear of the frame it belongs to. + */ + const media = }/>; const title = file.title || file.name || video.stem || video.name; let titleElm = diff --git a/app/src/components/ui/ui-layout.cy.js b/app/src/components/ui/ui-layout.cy.js index 143ecddd..be7348df 100644 --- a/app/src/components/ui/ui-layout.cy.js +++ b/app/src/components/ui/ui-layout.cy.js @@ -1,10 +1,11 @@ import React from 'react'; import { ActionInput, Button, Card, Header, Icon, IconButton, IconStack, Loading, MultiSelect, Panel, PathInput, - Message, Placeholder, Progress, Statistic, StatisticGroup, Status, TabBar, Table, + Message, Placeholder, Progress, SearchBox, Statistic, StatisticGroup, Status, TabBar, Table, tabClassName, TextInput, } from './index'; import {MemoryRouter} from 'react-router'; +import {VideoCard} from '../Videos'; import {CardPoster, contrastingColor, HelpHeader, LoadStatistic} from '../Common'; import {Notifications} from '@mantine/notifications'; import {clearToasts, toast} from './toast'; @@ -1916,3 +1917,106 @@ describe('a button and an icon button are the same height', () => { .to.be.at.least(34)); }); }); + +describe('corner chrome sits on the poster, not on the band around it', () => { + /* + * The tag mark and the video duration are pinned to a corner of "the poster". Once the + * poster gained a height cap, the painted image stopped filling the band it sits in: a + * portrait cover is 155px wide inside a 233px card, and a vertical video narrower still. + * The mark then floats in empty letterbox space nowhere near the image's corner. + * + * Geometry against a real painted image, so it belongs here. It has to be checked on a + * ratio that is HEIGHT-bound -- on a 16:9 poster the image fills the band and a mark + * pinned to either one lands in the same place, which is why this went unnoticed. + */ + const tagged = (width, height) => ({...posterFile(width, height), tags: [{name: 'ham radio'}]}); + + [ + {name: 'a portrait book cover', w: 306, h: 396}, + {name: 'a vertical video', w: 720, h: 1280}, + {name: 'a poster smaller than the card', w: 120, h: 90}, + ].forEach(({name, w, h}) => { + it(`pins the tag to the corner of ${name}`, () => { + servePoster(w, h); + cy.mountUI(narrowCard()); + + cy.get('.wrolpi-card-poster img') + .should(($img) => expect($img[0].naturalWidth, 'poster loaded').to.equal(w)) + .should(($img) => { + const image = $img[0].getBoundingClientRect(); + const band = $img[0].closest('.wrolpi-card-poster').getBoundingClientRect(); + // The premise: this ratio really does leave letterbox space to get lost in. + expect(band.width - image.width, `${name} is narrower than its band`) + .to.be.greaterThan(20); + + const mark = Cypress.$('.wrolpi-card-tag')[0].getBoundingClientRect(); + expect(mark.left, 'tag tracks the image, not the band') + .to.be.closeTo(image.left, 2); + expect(mark.top, 'tag sits at the top of the image').to.be.closeTo(image.top, 2); + }); + }); + }); + + it('keeps the tag inside the poster link, so clicking it still navigates', () => { + // The mark used to render inside the Link beside the image and was part of the + // poster's hit target; a sibling placed before the link is a dead spot on the corner. + servePoster(306, 396); + cy.mountUI(narrowCard()); + + cy.get('.wrolpi-card-tag').should(($tag) => + expect($tag[0].closest('a'), 'the tag is inside the link').to.not.be.null); + }); +}); + +describe('a video card pins its duration to the poster', () => { + /* + * The same defect as the tag mark, on the other corner and reached by a different + * route: the duration was pinned to a wrapper around CardPoster rather than to the + * poster itself. Mounted through the real VideoCard, because the wrapper was the + * subject and a hand-built poster would not have one. + */ + it('keeps the duration on a vertical video, not out in the band', () => { + servePoster(720, 1280); + // mountWithRouter, not mountUI: VideoCard reads the sort order from the query + // context, which the spartan mount deliberately does not provide. + cy.mountWithRouter(
); + + cy.get('.wrolpi-card-poster img') + .should(($img) => expect($img[0].naturalWidth, 'poster loaded').to.equal(720)) + .should(($img) => { + const image = $img[0].getBoundingClientRect(); + const band = $img[0].closest('.wrolpi-card-poster').getBoundingClientRect(); + expect(band.width - image.width, 'the poster is narrower than its band') + .to.be.greaterThan(20); + + const badge = Cypress.$('.duration-overlay')[0].getBoundingClientRect(); + expect(badge.right, 'the duration tracks the image, not the band') + .to.be.closeTo(image.right, 6); + }); + }); +}); + +describe('the search clear button matches the field it clears', () => { + /* + * Aligning IconButton with Button changed every icon button in the app, and this is the + * one in the library that stands beside an input rather than beside a button. It is + * also the case the remap was built for: `--ai-size-input-*` exists precisely so an + * ActionIcon can match an Input, so if this pair is ragged the remap picked the wrong + * scale. + */ + it('is exactly as tall as the search field', () => { + cy.mountUI( {}}/>); + + cy.get('.wrolpi-searchbox-clear').should(($clear) => { + const input = Cypress.$('input')[0].getBoundingClientRect(); + const clear = $clear[0].getBoundingClientRect(); + expect(input.height, 'the field has a height at all').to.be.greaterThan(10); + expect(clear.height, 'clear button vs search field').to.be.closeTo(input.height, 0.5); + }); + }); +}); diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index 191dd82c..d9115edd 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -1154,8 +1154,27 @@ html[data-theme="amber"] .wrolpi-progress-fill { --card-poster-max-height: 200px; } +/* + * The band is the full width of the card and centres the poster in it. The frame inside + * shrink-wraps the painted image, and is what corner chrome -- the tagged mark, a video's + * duration -- is positioned against. Pinning that chrome to the BAND puts it in the + * letterbox space beside a height-capped poster: 39px adrift on a book cover, 60px on a + * vertical video. + * + * The frame's own width falls out of the caps below. Its intrinsic contribution is the + * image's natural width already reduced by `max-height`, so a portrait cover contributes + * 155px and gets 155px; a 16:9 poster contributes 1280px and is flex-shrunk to the band, + * at which point `max-width: 100%` resolves against a width that is finally definite. + */ .wrolpi-card-poster { + display: flex; + justify-content: center; +} + +.wrolpi-card-poster-frame { position: relative; + display: block; + min-width: 0; } .wrolpi-card-poster img { @@ -1164,7 +1183,6 @@ html[data-theme="amber"] .wrolpi-progress-fill { height: auto; max-width: 100%; max-height: var(--card-poster-max-height); - margin: 0 auto; } .wrolpi-card-icon {