Fix spacing and tests from react-window Grid change

marlo-longley · marlo-longley · commit 0c57d4f87f0d · 2026-03-27T17:33:49.000-04:00
diff --git a/__tests__/src/components/CompanionWindowFactory.test.js b/__tests__/src/components/CompanionWindowFactory.test.js
@@ -62,7 +62,7 @@ describe('CompanionWindowFactory', () => {
         content: 'thumbnailNavigation',
       });
 
-      expect(screen.getByRole('grid')).toHaveAccessibleName('Thumbnails');
+      expect(screen.getByLabelText('Thumbnails')).toBeInTheDocument();
     });
   });
 
diff --git a/__tests__/src/components/ThumbnailCanvasGrouping.test.js b/__tests__/src/components/ThumbnailCanvasGrouping.test.js
@@ -38,10 +38,7 @@ describe('ThumbnailCanvasGrouping', () => {
     setCanvas = vi.fn();
     wrapper = createWrapper({ setCanvas });
   });
-  const spyCurrentCanvasClass = vi.spyOn(
-    ThumbnailCanvasGrouping.prototype,
-    'currentCanvasClass',
-  );
+  const spyCurrentCanvasClass = vi.spyOn(ThumbnailCanvasGrouping.prototype, 'currentCanvasClass');
   afterEach(() => {
     spyCurrentCanvasClass.mockClear();
   });
@@ -55,19 +52,15 @@ describe('ThumbnailCanvasGrouping', () => {
     wrapper.unmount();
     const user = userEvent.setup();
     wrapper = createWrapper({ index: 0, setCanvas });
-    await user.click(
-      wrapper.container.querySelector('.mirador-thumbnail-nav-canvas-0'), // eslint-disable-line testing-library/no-node-access
-    );
+    await user.click(wrapper.container.querySelector('.mirador-thumbnail-nav-canvas-0')); // eslint-disable-line testing-library/no-node-access
     expect(spyCurrentCanvasClass).toHaveBeenCalledWith([0]);
     expect(spyCurrentCanvasClass).toHaveReturnedWith('current-canvas-grouping');
-    expect(setCanvas).toHaveBeenCalledWith(
-      'http://iiif.io/api/presentation/2.0/example/fixtures/canvas/24/c1.json',
-    );
+    expect(setCanvas).toHaveBeenCalledWith('http://iiif.io/api/presentation/2.0/example/fixtures/canvas/24/c1.json');
   });
   describe('attributes based off far-bottom position', () => {
     it('in button div', () => {
       expect(screen.getByRole('button', { name: 'Image 1' })).toHaveStyle({
-        height: '123px',
+        height: '119px',
         width: 'auto',
       });
     });
diff --git a/__tests__/src/components/ThumbnailNavigation.test.js b/__tests__/src/components/ThumbnailNavigation.test.js
@@ -42,36 +42,49 @@ describe('ThumbnailNavigation', () => {
     expect(screen.getAllByRole('gridcell').length).toEqual(3);
   });
 
+  it('uses Grid component for horizontal (far-bottom) layout', () => {
+    render(<Subject position="far-bottom" />);
+    // Grid component renders with role="grid" inside the Thumbnails wrapper
+    const thumbnailWrapper = screen.getByLabelText('Thumbnails');
+    const grid = thumbnailWrapper.querySelector('[role="grid"]');
+    expect(grid).toBeInTheDocument();
+    // Grid should have 3 gridcells (all in one row)
+    expect(screen.getAllByRole('gridcell')).toHaveLength(3);
+  });
+
+  it('uses List component for vertical (far-right) layout', () => {
+    render(<Subject position="far-right" />);
+    // List component renders with role="list" inside the Thumbnails wrapper
+    const thumbnailWrapper = screen.getByLabelText('Thumbnails');
+    const list = thumbnailWrapper.querySelector('[role="list"]');
+    expect(list).toBeInTheDocument();
+    // List should have 3 gridcells (each as a separate item)
+    expect(screen.getAllByRole('gridcell')).toHaveLength(3);
+  });
+
   // TODO: Test that we recalculate dimensions when the view changes (resetAfterIndex)
   // TODO: Test that the window scrolls when the canvasIndex prop changes (scorllToItem)
 
   it('gives the grid a size', () => {
     const { rerender } = render(<Subject />);
-    expect(screen.getByRole('grid')).toHaveStyle({
-      height: '150px',
-      width: '100%',
-    });
+    expect(screen.getByLabelText('Thumbnails')).toHaveStyle({ height: '150px', width: '100%' });
 
     rerender(<Subject position="far-right" />);
-    expect(screen.getByRole('grid')).toHaveStyle({
-      height: '100%',
-      minHeight: 0,
-      width: '123px',
-    });
+    expect(screen.getByLabelText('Thumbnails')).toHaveStyle({ height: '100%', minHeight: 0, width: '127px' });
   });
 
   it('roughly doubles the width of the grid in book view', () => {
     const { rerender } = render(<Subject position="far-right" />);
-    expect(screen.getByRole('grid')).toHaveStyle({ width: '123px' });
+    expect(screen.getByLabelText('Thumbnails')).toHaveStyle({ width: '127px' });
 
     rerender(<Subject position="far-right" view="book" />);
-    expect(screen.getByRole('grid')).toHaveStyle({ width: '223px' });
+    expect(screen.getByLabelText('Thumbnails')).toHaveStyle({ width: '227px' });
   });
 
   it('calculates the scaled width of each cell', () => {
     render(<Subject />);
 
-    expect(screen.getAllByRole('gridcell')[0]).toHaveStyle({ width: '74px' });
+    expect(screen.getAllByRole('gridcell')[0]).toHaveStyle({ width: '111px' });
   });
 
   it('calculates the scaled height of each cell when on the right', () => {
@@ -82,7 +95,7 @@ describe('ThumbnailNavigation', () => {
   it('keeps a minimum size for each cell', () => {
     render(<Subject fixture={zeroWidthFixture} />);
 
-    expect(screen.getAllByRole('gridcell')[0]).toHaveStyle({ width: '100px' });
+    expect(screen.getAllByRole('gridcell')[0]).toHaveStyle({ width: '111px' });
   });
 
   it('keeps a minimum size for each cell when on the right', () => {
@@ -97,69 +110,31 @@ describe('ThumbnailNavigation', () => {
 
     describe('handleKeyUp', () => {
       it('handles right arrow by advancing the current canvas', async () => {
-        render(
-          <Subject
-            canvasIndex={1}
-            hasNextCanvas
-            setNextCanvas={setNextCanvas}
-          />,
-        );
+        render(<Subject canvasIndex={1} hasNextCanvas setNextCanvas={setNextCanvas} />);
 
         screen.getByRole('grid').focus();
-        fireEvent.keyDown(screen.getByRole('grid'), {
-          code: 'ArrowRight',
-          key: 'ArrowRight',
-        });
+        fireEvent.keyDown(screen.getByRole('grid'), { code: 'ArrowRight', key: 'ArrowRight' });
         expect(setNextCanvas).toHaveBeenCalled();
       });
       it('handles down arrow by advancing the current canvas when the canvas is on the right', () => {
-        render(
-          <Subject
-            canvasIndex={1}
-            hasNextCanvas
-            position="far-right"
-            setNextCanvas={setNextCanvas}
-          />,
-        );
+        render(<Subject canvasIndex={1} hasNextCanvas position="far-right" setNextCanvas={setNextCanvas} />);
 
-        screen.getByRole('grid').focus();
-        fireEvent.keyDown(screen.getByRole('grid'), {
-          code: 'ArrowDown',
-          key: 'ArrowDown',
-        });
+        screen.getByLabelText('Thumbnails').focus();
+        fireEvent.keyDown(screen.getByLabelText('Thumbnails'), { code: 'ArrowDown', key: 'ArrowDown' });
         expect(setNextCanvas).toHaveBeenCalled();
       });
       it('handles left arrow by selecting the previous canvas', () => {
-        render(
-          <Subject
-            canvasIndex={2}
-            hasPreviousCanvas
-            setPreviousCanvas={setPreviousCanvas}
-          />,
-        );
+        render(<Subject canvasIndex={2} hasPreviousCanvas setPreviousCanvas={setPreviousCanvas} />);
 
-        screen.getByRole('grid').focus();
-        fireEvent.keyDown(screen.getByRole('grid'), {
-          code: 'ArrowLeft',
-          key: 'ArrowLeft',
-        });
+        screen.getByLabelText('Thumbnails').focus();
+        fireEvent.keyDown(screen.getByLabelText('Thumbnails'), { code: 'ArrowLeft', key: 'ArrowLeft' });
         expect(setPreviousCanvas).toHaveBeenCalled();
       });
       it('handles up arrow by selecting the previous canvas when the canvas is on the right', () => {
-        render(
-          <Subject
-            canvasIndex={2}
-            hasPreviousCanvas
-            position="far-right"
-            setPreviousCanvas={setPreviousCanvas}
-          />,
-        );
+        render(<Subject canvasIndex={2} hasPreviousCanvas position="far-right" setPreviousCanvas={setPreviousCanvas} />);
 
-        screen.getByRole('grid').focus();
-        fireEvent.keyDown(screen.getByRole('grid'), {
-          code: 'ArrowUp',
-          key: 'ArrowUp',
-        });
+        screen.getByLabelText('Thumbnails').focus();
+        fireEvent.keyDown(screen.getByLabelText('Thumbnails'), { code: 'ArrowUp', key: 'ArrowUp' });
         expect(setPreviousCanvas).toHaveBeenCalled();
       });
     });
diff --git a/src/components/ThumbnailCanvasGrouping.jsx b/src/components/ThumbnailCanvasGrouping.jsx
@@ -53,8 +53,8 @@ export class ThumbnailCanvasGrouping extends PureComponent {
     const currentGroupings = canvasGroupings[itemIndex];
     const SPACING = 12;
 
-    // In react-window v2 horizontal lists, width is not provided in style
-    // The height value in style is actually the width for horizontal lists
+    // In react-window v2 Grid (horizontal layout), columnWidth is passed as style.height
+    // For List (vertical layout), rowHeight is passed as style.height
     const isHorizontal = position === 'far-bottom';
     let calculatedWidth = null;
     if (isHorizontal && style.height) {
@@ -72,9 +72,7 @@ export class ThumbnailCanvasGrouping extends PureComponent {
         style={{
           ...style,
           boxSizing: 'content-box',
-          height: Number.isInteger(style.height)
-            ? style.height - SPACING
-            : null,
+          height: Number.isInteger(style.height) ? style.height - SPACING : null,
           left: Number.isInteger(style.left) ? style.left + SPACING / 2 : null,
           padding: SPACING / 2,
           top: Number.isInteger(style.top) ? style.top + SPACING / 2 : null,
@@ -109,25 +107,15 @@ export class ThumbnailCanvasGrouping extends PureComponent {
             width: position === 'far-bottom' ? 'auto' : `${style.width}px`,
           })}
           className={classNames(
-            ns([
-              'thumbnail-nav-canvas',
-              `thumbnail-nav-canvas-${itemIndex}`,
-              this.currentCanvasClass(
-                currentGroupings.map(canvas => canvas.index),
-              ),
-            ]),
+            ns(['thumbnail-nav-canvas', `thumbnail-nav-canvas-${itemIndex}`, this.currentCanvasClass( currentGroupings.map(canvas => canvas.index))]),
           )}
         >
           {currentGroupings.map((canvas, i) => (
             <IIIFThumbnail
               key={canvas.id}
               resource={canvas}
               labelled={showThumbnailLabels}
-              maxHeight={
-                position === 'far-right'
-                  ? style.height - 1.5 * SPACING
-                  : height - 1.5 * SPACING
-              }
+              maxHeight={(position === 'far-right') ? style.height - (1.5 * SPACING) : height - (1.5 * SPACING)}
               variant="inside"
             />
           ))}
diff --git a/src/components/ThumbnailNavigation.jsx b/src/components/ThumbnailNavigation.jsx
@@ -24,7 +24,7 @@ export function ThumbnailNavigation({
 }) {
   const { t } = useTranslation();
   const scrollbarSize = 15;
-  const spacing = 8; // 2 * (2px margin + 2px border + 2px padding + 2px padding)
+  const spacing = 12; // 2 * (2px margin + 2px border + 2px padding + 2px padding)
   const gridRef = useRef();
   const previousView = useRef(view);
   const canvasWorlds = useCanvasWorldService();
@@ -68,7 +68,7 @@ export function ThumbnailNavigation({
   }, [canvasIndex]);
 
   /** */
-  const handleKeyDown = e => {
+  const handleKeyDown = (e) => {
     let nextKey = 'ArrowRight';
     let previousKey = 'ArrowLeft';
     if (position === 'far-right') {
@@ -91,7 +91,7 @@ export function ThumbnailNavigation({
    * When on right, row height
    * When on bottom, column width
    */
-  const calculateScaledSize = index => {
+  const calculateScaledSize = (index) => {
     const canvases = canvasGroupings[index];
     if (!canvases) return thumbnailNavigation.width + spacing;
 
@@ -100,7 +100,7 @@ export function ThumbnailNavigation({
     switch (position) {
       case 'far-right': {
         const calc = Math.floor(
-          (calculatingWidth(canvases.length) * bounds[3]) / bounds[2],
+          calculatingWidth(canvases.length) * bounds[3] / bounds[2]
         );
         if (!Number.isInteger(calc)) return thumbnailNavigation.width + spacing;
         return calc + spacing;
@@ -109,17 +109,16 @@ export function ThumbnailNavigation({
       default: {
         if (bounds[3] === 0) return thumbnailNavigation.width + spacing;
         const calc = Math.ceil(
-          ((thumbnailNavigation.height - scrollbarSize - spacing - 4)
-            * bounds[2])
-            / bounds[3],
+          (thumbnailNavigation.height - scrollbarSize - spacing - 4)
+           * bounds[2] / bounds[3],
         );
         return calc;
       }
     }
   };
 
   /** */
-  const calculatingWidth = canvasesLength => {
+  const calculatingWidth = (canvasesLength) => {
     if (canvasesLength === 1) {
       return thumbnailNavigation.width;
     }
@@ -128,9 +127,7 @@ export function ThumbnailNavigation({
 
   /** */
   const style = useCallback(() => {
-    const width = view === 'book'
-      ? thumbnailNavigation.width * 2
-      : thumbnailNavigation.width;
+    const width = view === 'book' ? thumbnailNavigation.width * 2 : thumbnailNavigation.width;
 
     switch (position) {
       case 'far-right':
@@ -172,7 +169,9 @@ export function ThumbnailNavigation({
   };
   return (
     <Paper
-      className={classNames(ns('thumb-navigation'))}
+      className={classNames(
+        ns('thumb-navigation'),
+      )}
       sx={{
         '&:focus': {
           boxShadow: 0,
@@ -185,10 +184,9 @@ export function ThumbnailNavigation({
       style={style()}
       tabIndex={0}
       onKeyDown={handleKeyDown}
-      role="grid"
       ref={paperRef}
     >
-      <div role="row" style={{ height: '100%', width: '100%' }}>
+      <div style={{ height: '100%', width: '100%' }}>
         {canvasGroupings.length > 0 && position === 'far-bottom' && (
           <Grid
             columnCount={canvasGroupings.length}
