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 {