Skip to content

Commit

Permalink
address review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
joshuadkitenge committed Dec 6, 2024
1 parent 040390d commit 9dadad5
Show file tree
Hide file tree
Showing 12 changed files with 63 additions and 44 deletions.
4 changes: 2 additions & 2 deletions cypress/e2e/with_mock_data/items.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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');
Expand Down
2 changes: 1 addition & 1 deletion src/api/api.types.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,6 @@ export interface APIImage
thumbnail_base64: string;
}

export interface ImageGet extends APIImage {
export interface APIImageWithURL extends APIImage {
url: string;
}
12 changes: 12 additions & 0 deletions src/api/images.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
11 changes: 5 additions & 6 deletions src/api/images.tsx
Original file line number Diff line number Diff line change
@@ -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),
});
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion src/common/images/galleryLightbox.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down Expand Up @@ -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
Expand Down
16 changes: 15 additions & 1 deletion src/common/images/imageGallery.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ const ImageGallery = (props: ImageGalleryProps) => {
pagination: { pageSize: 16, pageIndex: 0 },
},
storeInUrl: true,
paginationOnly: true,
urlParamName: 'imageState',
});

Expand Down Expand Up @@ -326,13 +325,28 @@ 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}
item
container
xs
key={`thumbnail-displayed-${index}`}
style={{
maxWidth:
images.length === 1 ||
(images.length % preservedState.pagination.pageSize ===
1 &&
isLastPage)
? '50%'

Check warning on line 347 in src/common/images/imageGallery.component.tsx

View check run for this annotation

Codecov / codecov/patch

src/common/images/imageGallery.component.tsx#L347

Added line #L347 was not covered by tests
: undefined,
}}
minWidth={'350px'}
>
<Grid
Expand Down
3 changes: 2 additions & 1 deletion src/common/images/imageInformationDialog.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions src/common/images/thumbnailImage.component.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});

Expand All @@ -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);
Expand Down
42 changes: 16 additions & 26 deletions src/common/images/thumbnailImage.component.tsx
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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)}
/>
);
};

Expand Down
2 changes: 1 addition & 1 deletion src/common/preservedTableState.component.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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) =>
Expand Down
6 changes: 4 additions & 2 deletions src/mocks/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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',
Expand All @@ -1056,7 +1058,7 @@ export const handlers = [
}
}

if (Number(id) === 5) {
if (id === '5') {
return HttpResponse.error();
}

Expand Down

0 comments on commit 9dadad5

Please sign in to comment.