Carousel paints its own markup instead of going through PhotoCard, so it inherited none of the colour-label treatment — not other viewers' marks from #1178 and not the viewer's own from #1044. A photo the client flagged green looked identical to one nobody had touched, in a layout a photographer can select like any other. It has been missing since the feature landed. Rendered in both places the carousel paints a photo. The thumbnail strip is the one that matters: it is the only place the layout shows more than one photo at a time, so it is the only place a label can actually be scanned. Two small additions to ColorLabelBadge, both defaulting to today's behaviour so every existing layout renders byte-identically: - `size="sm"` shrinks the dots for the strip's 80px tiles, where the grid-sized 20px dot plus three 10px ones covers most of the image. - `position` is overridable because this layout has different corners free. Its top-left carries the counter and category chips and its top-right the play/fullscreen buttons, so the badge goes bottom-left on the main frame — the only corner left — and top-left in the strip, where nothing competes. This is the per-layout position override #1178's review said would start to pay for itself the first time a layout genuinely needed a different corner. Co-authored-by: Paul Nothaft <[email protected]>
This commit is contained in:
co-authored by
Paul Nothaft
parent
d3e9a7cf0d
commit
da802169a8
@@ -13,6 +13,19 @@ interface ColorLabelBadgeProps {
|
||||
otherColorLabels?: string[];
|
||||
/** Extra classes for positioning inside the tile. */
|
||||
className?: string;
|
||||
/**
|
||||
* Dot scale. 'sm' is for surfaces smaller than a grid tile — the carousel's
|
||||
* 80px thumbnail strip (#1189), where the default 20px dot plus three 10px
|
||||
* ones covers most of the image.
|
||||
*/
|
||||
size?: 'md' | 'sm';
|
||||
/**
|
||||
* Where the dot row sits inside its positioned ancestor. Overridable because
|
||||
* the tile layouts and the carousel have different corners free: the
|
||||
* carousel's own top-left carries its counter and category chips, so the
|
||||
* default would land underneath them (#1189).
|
||||
*/
|
||||
position?: string;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -27,6 +40,8 @@ export const ColorLabelBadge: React.FC<ColorLabelBadgeProps> = ({
|
||||
colorLabel,
|
||||
otherColorLabels = [],
|
||||
className = '',
|
||||
size = 'md',
|
||||
position = 'top-2 left-2',
|
||||
}) => {
|
||||
const { t } = useTranslation();
|
||||
|
||||
@@ -46,6 +61,9 @@ export const ColorLabelBadge: React.FC<ColorLabelBadgeProps> = ({
|
||||
colors: others.map((c) => t(`feedback.colorLabels.${c}`, c)).join(', '),
|
||||
});
|
||||
|
||||
const mineDotClass = size === 'sm' ? 'w-3.5 h-3.5 border' : 'w-5 h-5 border-2';
|
||||
const otherDotClass = size === 'sm' ? 'w-2 h-2' : 'w-2.5 h-2.5';
|
||||
|
||||
return (
|
||||
<>
|
||||
{mine && swatch && (
|
||||
@@ -64,10 +82,10 @@ export const ColorLabelBadge: React.FC<ColorLabelBadgeProps> = ({
|
||||
Masonry a media-type badge — and anything placed there gets painted
|
||||
over. Sharing this position also reads better: your mark and everyone
|
||||
else's are the same kind of information. */}
|
||||
<span className="absolute top-2 left-2 pointer-events-none flex items-center gap-1">
|
||||
<span className={`absolute ${position} pointer-events-none flex items-center gap-1`}>
|
||||
{mine && swatch && (
|
||||
<span
|
||||
className="flex items-center justify-center w-5 h-5 rounded-full border-2 border-white/90 shadow"
|
||||
className={`flex items-center justify-center ${mineDotClass} rounded-full border-white/90 shadow`}
|
||||
style={{ backgroundColor: swatch.fill }}
|
||||
// Colour alone can't carry the meaning — the accessible name does.
|
||||
title={t('feedback.markedAs', 'Marked as {{color}}', { color: name })}
|
||||
@@ -85,7 +103,7 @@ export const ColorLabelBadge: React.FC<ColorLabelBadgeProps> = ({
|
||||
{others.map((c) => (
|
||||
<span
|
||||
key={c}
|
||||
className="block w-2.5 h-2.5 rounded-full border border-white/90 shadow-sm"
|
||||
className={`block ${otherDotClass} rounded-full border border-white/90 shadow-sm`}
|
||||
style={{ backgroundColor: COLOR_LABEL_SWATCHES[c].fill }}
|
||||
/>
|
||||
))}
|
||||
|
||||
@@ -0,0 +1,79 @@
|
||||
/**
|
||||
* Colour labels in the Carousel layout (#1189).
|
||||
*
|
||||
* Carousel paints its own markup instead of going through PhotoCard, so it
|
||||
* inherited none of the colour-label treatment — not other viewers' marks from
|
||||
* #1178 and not the viewer's own from #1044. A photo the client flagged green
|
||||
* looked identical to one nobody had touched, in a layout a photographer can
|
||||
* select like any other.
|
||||
*
|
||||
* The strip is what these tests care about most: it is the only place the
|
||||
* carousel shows more than one photo at once, so it is the only place a label
|
||||
* can actually be scanned.
|
||||
*/
|
||||
|
||||
import React from 'react';
|
||||
import { render, screen } from '@testing-library/react';
|
||||
import { describe, it, expect } from 'vitest';
|
||||
import { ColorLabelBadge } from '../ColorLabelBadge';
|
||||
|
||||
describe('ColorLabelBadge sizing and placement (#1189)', () => {
|
||||
const dots = (container: HTMLElement) =>
|
||||
Array.from(container.querySelectorAll('span')).filter((el) =>
|
||||
el.className.includes('rounded-full')
|
||||
);
|
||||
|
||||
it('defaults are untouched, so every existing layout renders as before', () => {
|
||||
const { container } = render(<ColorLabelBadge colorLabel="green" otherColorLabels={['red']} />);
|
||||
const row = container.querySelector('.absolute.top-2.left-2');
|
||||
expect(row).toBeTruthy();
|
||||
|
||||
const [own] = dots(container);
|
||||
expect(own.className).toContain('w-5');
|
||||
expect(own.className).toContain('border-2');
|
||||
});
|
||||
|
||||
it('the small variant shrinks both the own dot and the others', () => {
|
||||
// The carousel strip is 80px square; the grid-sized dots cover most of it.
|
||||
const { container } = render(
|
||||
<ColorLabelBadge colorLabel="green" otherColorLabels={['red', 'blue']} size="sm" />
|
||||
);
|
||||
const [own, ...others] = dots(container);
|
||||
expect(own.className).toContain('w-3.5');
|
||||
expect(own.className).not.toContain('w-5');
|
||||
others.forEach((d) => expect(d.className).toContain('w-2'));
|
||||
});
|
||||
|
||||
it('the position is overridable, because the carousel has different corners free', () => {
|
||||
// Its top-left carries the counter and category chips, so the default
|
||||
// would render underneath them.
|
||||
const { container } = render(
|
||||
<ColorLabelBadge colorLabel="green" position="bottom-4 left-4" />
|
||||
);
|
||||
expect(container.querySelector('.absolute.bottom-4.left-4')).toBeTruthy();
|
||||
expect(container.querySelector('.absolute.top-2.left-2')).toBeNull();
|
||||
});
|
||||
|
||||
it('still renders nothing when there is no label at all', () => {
|
||||
// The carousel maps over every photo in the strip, so an unmarked photo
|
||||
// must add no markup rather than an empty positioned span.
|
||||
const { container } = render(
|
||||
<ColorLabelBadge colorLabel={null} otherColorLabels={[]} size="sm" position="top-1 left-1" />
|
||||
);
|
||||
expect(container.innerHTML).toBe('');
|
||||
});
|
||||
|
||||
it('shows other viewers marks even when the viewer has none of their own', () => {
|
||||
// The #1178 case, which is the one that makes the carousel gap visible: a
|
||||
// client marked it, the photographer has not.
|
||||
render(<ColorLabelBadge colorLabel={null} otherColorLabels={['green']} size="sm" />);
|
||||
expect(screen.getByRole('img', { name: /also marked by others/i })).toBeTruthy();
|
||||
});
|
||||
|
||||
it('keeps the inset ring, so the selected photo still reads at a glance', () => {
|
||||
const { container } = render(<ColorLabelBadge colorLabel="red" size="sm" />);
|
||||
const ring = container.querySelector('.absolute.inset-0');
|
||||
expect(ring).toBeTruthy();
|
||||
expect((ring as HTMLElement).style.boxShadow).toContain('inset');
|
||||
});
|
||||
});
|
||||
@@ -2,6 +2,7 @@ import React, { useState, useEffect, useRef } from 'react';
|
||||
import { ChevronLeft, ChevronRight, Download, Maximize2, Play, Pause, Heart, MessageSquare } from 'lucide-react';
|
||||
import { useTheme } from '../../../contexts/ThemeContext';
|
||||
import { AuthenticatedImage, Button } from '../../common';
|
||||
import { ColorLabelBadge } from '../ColorLabelBadge';
|
||||
import type { BaseGalleryLayoutProps } from './BaseGalleryLayout';
|
||||
import { FeedbackIdentityModal } from '../../gallery/FeedbackIdentityModal';
|
||||
import { feedbackService } from '../../../services/feedback.service';
|
||||
@@ -90,7 +91,17 @@ export const CarouselGalleryLayout: React.FC<BaseGalleryLayoutProps> = ({
|
||||
isGallery={true}
|
||||
protectFromDownload={!allowDownloads}
|
||||
/>
|
||||
|
||||
|
||||
{/* Colour labels for the photo in view (#1189). Bottom-left because it
|
||||
is the only corner this layout leaves free — top-left carries the
|
||||
counter and category chips, top-right the play/fullscreen buttons,
|
||||
and both sides the prev/next controls. */}
|
||||
<ColorLabelBadge
|
||||
colorLabel={currentPhoto.my_color_label}
|
||||
otherColorLabels={currentPhoto.other_color_labels}
|
||||
position="bottom-4 left-4"
|
||||
/>
|
||||
|
||||
{/* Navigation Controls */}
|
||||
<div className="absolute inset-0 flex items-center justify-between p-4">
|
||||
<button
|
||||
@@ -258,6 +269,16 @@ export const CarouselGalleryLayout: React.FC<BaseGalleryLayoutProps> = ({
|
||||
isGallery={true}
|
||||
protectFromDownload={!allowDownloads}
|
||||
/>
|
||||
{/* The strip is the only place this layout shows more than one
|
||||
photo at a time, so it is the only place a label can
|
||||
actually be scanned (#1189). Small variant: these tiles are
|
||||
80px, where the grid-sized dots cover most of the image. */}
|
||||
<ColorLabelBadge
|
||||
colorLabel={photo.my_color_label}
|
||||
otherColorLabels={photo.other_color_labels}
|
||||
size="sm"
|
||||
position="top-1 left-1"
|
||||
/>
|
||||
</button>
|
||||
))}
|
||||
</div>
|
||||
|
||||
Reference in New Issue
Block a user