From 9dadad58b9f52edc70f404e87f02be9f71ae8212 Mon Sep 17 00:00:00 2001
From: Joshua Kitenge <joshua.kitenge@stfc.ac.uk>
Date: Thu, 5 Dec 2024 15:25:22 +0000
Subject: [PATCH] address review comments

---
 cypress/e2e/with_mock_data/items.cy.ts        |  4 +-
 src/api/api.types.tsx                         |  2 +-
 src/api/images.test.tsx                       | 12 ++++++
 src/api/images.tsx                            | 11 +++--
 ...eInformationDialog.component.test.tsx.snap |  2 +-
 .../images/galleryLightbox.component.tsx      |  3 +-
 src/common/images/imageGallery.component.tsx  | 16 ++++++-
 .../imageInformationDialog.component.tsx      |  3 +-
 .../images/thumbnailImage.component.test.tsx  |  4 +-
 .../images/thumbnailImage.component.tsx       | 42 +++++++------------
 src/common/preservedTableState.component.tsx  |  2 +-
 src/mocks/handlers.ts                         |  6 ++-
 12 files changed, 63 insertions(+), 44 deletions(-)

diff --git a/cypress/e2e/with_mock_data/items.cy.ts b/cypress/e2e/with_mock_data/items.cy.ts
index d9a3ca47b..9ef8875b6 100644
--- a/cypress/e2e/with_mock_data/items.cy.ts
+++ b/cypress/e2e/with_mock_data/items.cy.ts
@@ -707,7 +707,7 @@ describe('Items', () => {
       ).should('exist');
 
       cy.findByText('Gallery').click();
-      cy.findByText('The image cannot be loaded').should('exist');
+      cy.findByAltText('The image cannot be loaded').should('exist');
     });
 
     it('displays and hides filters, applies and clears name filter on gallery view', () => {
@@ -800,7 +800,7 @@ describe('Items', () => {
 
       cy.findByText('Gallery').click();
 
-      cy.findByText('The image cannot be loaded').click();
+      cy.findByAltText('The image cannot be loaded').click();
       cy.findByTestId('galleryLightBox').within(() => {
         cy.findByText('File name: stfc-logo-blue-text.png').should('exist');
         cy.findByText('Title: stfc-logo-blue-text').should('exist');
diff --git a/src/api/api.types.tsx b/src/api/api.types.tsx
index 3e3089929..1cbd1dd83 100644
--- a/src/api/api.types.tsx
+++ b/src/api/api.types.tsx
@@ -258,6 +258,6 @@ export interface APIImage
   thumbnail_base64: string;
 }
 
-export interface ImageGet extends APIImage {
+export interface APIImageWithURL extends APIImage {
   url: string;
 }
diff --git a/src/api/images.test.tsx b/src/api/images.test.tsx
index 164cb41e6..e4a20b8ce 100644
--- a/src/api/images.test.tsx
+++ b/src/api/images.test.tsx
@@ -32,6 +32,18 @@ describe('images api functions', () => {
 
       expect(result.current.data?.length).toEqual(1);
     });
+
+    it('sends request to fetch primary image data and returns successful empty list response', async () => {
+      const { result } = renderHook(() => useGetImages('90', true), {
+        wrapper: hooksWrapperWithProviders(),
+      });
+
+      await waitFor(() => {
+        expect(result.current.isSuccess).toBeTruthy();
+      });
+
+      expect(result.current.data?.length).toEqual(0);
+    });
   });
 
   describe('useGetImage', () => {
diff --git a/src/api/images.tsx b/src/api/images.tsx
index 60ff94874..e876c6441 100644
--- a/src/api/images.tsx
+++ b/src/api/images.tsx
@@ -1,21 +1,20 @@
 import { useQuery, UseQueryResult } from '@tanstack/react-query';
 import { AxiosError } from 'axios';
 import { storageApi } from './api';
-import { APIImage, ImageGet } from './api.types';
+import { APIImage, APIImageWithURL } from './api.types';
 
-export const getImage = async (id: string): Promise<ImageGet> => {
+export const getImage = async (id: string): Promise<APIImageWithURL> => {
   return storageApi.get(`/images/${id}`).then((response) => {
     return response.data;
   });
 };
 
 export const useGetImage = (
-  id?: string
-): UseQueryResult<ImageGet, AxiosError> => {
+  id: string
+): UseQueryResult<APIImageWithURL, AxiosError> => {
   return useQuery({
     queryKey: ['Image', id],
-    queryFn: () => getImage(id ?? ''),
-    enabled: !!id,
+    queryFn: () => getImage(id),
   });
 };
 
diff --git a/src/common/images/__snapshots__/imageInformationDialog.component.test.tsx.snap b/src/common/images/__snapshots__/imageInformationDialog.component.test.tsx.snap
index 4fffa0e39..a836d92bc 100644
--- a/src/common/images/__snapshots__/imageInformationDialog.component.test.tsx.snap
+++ b/src/common/images/__snapshots__/imageInformationDialog.component.test.tsx.snap
@@ -8,7 +8,7 @@ exports[`Image Information dialog Component > renders dialog correctly 1`] = `
     aria-hidden="true"
   />
   <div
-    class="MuiDialog-root MuiModal-root css-eodhl2-MuiModal-root-MuiDialog-root"
+    class="MuiDialog-root MuiModal-root css-1r9csp7-MuiModal-root-MuiDialog-root"
     role="presentation"
   >
     <div
diff --git a/src/common/images/galleryLightbox.component.tsx b/src/common/images/galleryLightbox.component.tsx
index 8cc182089..89485cbdc 100644
--- a/src/common/images/galleryLightbox.component.tsx
+++ b/src/common/images/galleryLightbox.component.tsx
@@ -57,6 +57,7 @@ const GalleryLightBox = (props: GalleryLightBoxProps) => {
     <Backdrop
       sx={{
         color: 'white',
+        // SciGateway navigation drawer is 1200, 1201 is the gallery lightbox
         zIndex: 1210 + 1,
         display: 'flex',
         alignItems: 'center',
@@ -176,7 +177,7 @@ const GalleryLightBox = (props: GalleryLightBoxProps) => {
                 The image cannot be loaded
               </Typography>
             )}
-            {!isLoading && data?.url && !hasError && (
+            {!isLoading && data?.url && !(hasError === data?.id) && (
               <img
                 // The key forces React to remount the <img> tag when hasError changes.
                 // This is necessary because, without remounting, the <img> tag doesn't
diff --git a/src/common/images/imageGallery.component.tsx b/src/common/images/imageGallery.component.tsx
index 08b3a457b..ffc1131a4 100644
--- a/src/common/images/imageGallery.component.tsx
+++ b/src/common/images/imageGallery.component.tsx
@@ -61,7 +61,6 @@ const ImageGallery = (props: ImageGalleryProps) => {
       pagination: { pageSize: 16, pageIndex: 0 },
     },
     storeInUrl: true,
-    paginationOnly: true,
     urlParamName: 'imageState',
   });
 
@@ -326,6 +325,12 @@ const ImageGallery = (props: ImageGalleryProps) => {
                   (img) => img.id === card.row.original.id
                 );
 
+                const lastPageIndex = Math.floor(
+                  images.length / preservedState.pagination.pageSize
+                );
+                const isLastPage =
+                  preservedState.pagination.pageIndex === lastPageIndex;
+
                 return isUndisplayed ? null : (
                   <Card
                     component={Grid}
@@ -333,6 +338,15 @@ const ImageGallery = (props: ImageGalleryProps) => {
                     container
                     xs
                     key={`thumbnail-displayed-${index}`}
+                    style={{
+                      maxWidth:
+                        images.length === 1 ||
+                        (images.length % preservedState.pagination.pageSize ===
+                          1 &&
+                          isLastPage)
+                          ? '50%'
+                          : undefined,
+                    }}
                     minWidth={'350px'}
                   >
                     <Grid
diff --git a/src/common/images/imageInformationDialog.component.tsx b/src/common/images/imageInformationDialog.component.tsx
index 859093e27..79ad5700f 100644
--- a/src/common/images/imageInformationDialog.component.tsx
+++ b/src/common/images/imageInformationDialog.component.tsx
@@ -23,7 +23,8 @@ const ImageInformationDialog = (props: ImageInformationDialogProps) => {
 
   return (
     <Dialog
-      sx={{ zIndex: 1210 + 2 }}
+      // SciGateway navigation drawer is 1200 and the gallery lightbox 1210
+      sx={{ zIndex: 1210 + 10 }}
       open={open}
       maxWidth="sm"
       disableEnforceFocus
diff --git a/src/common/images/thumbnailImage.component.test.tsx b/src/common/images/thumbnailImage.component.test.tsx
index 71a7f15a9..f70d66a5c 100644
--- a/src/common/images/thumbnailImage.component.test.tsx
+++ b/src/common/images/thumbnailImage.component.test.tsx
@@ -45,7 +45,7 @@ describe('ThumbnailImage Component', () => {
     );
     fireEvent.error(imageElement);
 
-    const fallbackText = screen.getByText('The image cannot be loaded');
+    const fallbackText = screen.getByAltText('The image cannot be loaded');
     expect(fallbackText).toBeInTheDocument();
   });
 
@@ -68,7 +68,7 @@ describe('ThumbnailImage Component', () => {
     );
     fireEvent.error(imageElement);
 
-    const fallbackText = screen.getByText('The image cannot be loaded');
+    const fallbackText = screen.getByAltText('The image cannot be loaded');
     fireEvent.click(fallbackText);
 
     expect(onClick).toHaveBeenCalledTimes(1);
diff --git a/src/common/images/thumbnailImage.component.tsx b/src/common/images/thumbnailImage.component.tsx
index f22a5cc3e..8ea658e0b 100644
--- a/src/common/images/thumbnailImage.component.tsx
+++ b/src/common/images/thumbnailImage.component.tsx
@@ -1,4 +1,4 @@
-import { Box, Typography } from '@mui/material';
+import { Box } from '@mui/material';
 import React from 'react';
 import { APIImage } from '../../api/api.types';
 
@@ -11,31 +11,21 @@ const ThumbnailImage = (props: ThumbnailImageProps) => {
   const { onClick, image } = props;
   const [hasError, setHasError] = React.useState<string | undefined>(undefined);
   return (
-    <>
-      {hasError === image.id ? (
-        <Typography
-          variant="body2"
-          color="textSecondary"
-          textAlign="center"
-          onClick={onClick}
-          sx={{ cursor: onClick ? 'pointer' : undefined }}
-        >
-          The image cannot be loaded
-        </Typography>
-      ) : (
-        <Box
-          component="img"
-          src={`data:image/webp;base64,${image.thumbnail_base64}`}
-          alt={image.description ?? 'No photo description available.'}
-          style={{
-            borderRadius: '4px',
-            cursor: onClick ? 'pointer' : undefined,
-          }}
-          onClick={onClick}
-          onError={() => setHasError(image.id)}
-        />
-      )}
-    </>
+    <Box
+      component="img"
+      src={`data:image/webp;base64,${image.thumbnail_base64}`}
+      alt={
+        hasError
+          ? 'The image cannot be loaded'
+          : (image.description ?? 'No photo description available.')
+      }
+      style={{
+        borderRadius: '4px',
+        cursor: onClick ? 'pointer' : undefined,
+      }}
+      onClick={onClick}
+      onError={() => setHasError(image.id)}
+    />
   );
 };
 
diff --git a/src/common/preservedTableState.component.tsx b/src/common/preservedTableState.component.tsx
index 4770c3814..acc66de45 100644
--- a/src/common/preservedTableState.component.tsx
+++ b/src/common/preservedTableState.component.tsx
@@ -46,7 +46,7 @@ interface StateSearchParams extends StatePartial {
 
 /* This matches the definition found in tanstack table (couldn't be directly imported
    as its a dependency of MRT) */
-export type Updater<T> = T | ((old: T) => T);
+type Updater<T> = T | ((old: T) => T);
 
 /* Returns correctly types value from an updater */
 const getValueFromUpdater = <T,>(updater: Updater<T>, currentValue: T) =>
diff --git a/src/mocks/handlers.ts b/src/mocks/handlers.ts
index 8903ac502..5c2b9e798 100644
--- a/src/mocks/handlers.ts
+++ b/src/mocks/handlers.ts
@@ -1034,6 +1034,8 @@ export const handlers = [
 
   http.get('/images/:id', ({ params }) => {
     const { id } = params;
+    // This is needed otherwise the msw would intercept the
+    // mocked image get request for the object store
     if (!isNaN(Number(id))) {
       let image = undefined;
       if (Number(id) % 2 === 0) {
@@ -1042,7 +1044,7 @@ export const handlers = [
           url: `${window.location.origin}/logo192.png?text=${encodeURIComponent(id as string)}`,
         };
       } else {
-        if (Number(id) === 3) {
+        if (id === '3') {
           image = {
             ...ImagesJSON[1],
             url: 'invalid url',
@@ -1056,7 +1058,7 @@ export const handlers = [
         }
       }
 
-      if (Number(id) === 5) {
+      if (id === '5') {
         return HttpResponse.error();
       }