From d31e17894ce3f8f6be82f426de879816d76acd97 Mon Sep 17 00:00:00 2001 From: Alexandre Vryghem Date: Fri, 26 Jul 2024 13:57:47 +0200 Subject: [PATCH 1/5] 116728: Removed unnecessary *ngVars from the ThumbnailComponent --- src/app/thumbnail/thumbnail.component.html | 20 ++++++------- src/app/thumbnail/thumbnail.component.spec.ts | 28 +++++++++---------- src/app/thumbnail/thumbnail.component.ts | 14 +++++----- 3 files changed, 30 insertions(+), 32 deletions(-) diff --git a/src/app/thumbnail/thumbnail.component.html b/src/app/thumbnail/thumbnail.component.html index 6a0516b0d4..049a47e1b0 100644 --- a/src/app/thumbnail/thumbnail.component.html +++ b/src/app/thumbnail/thumbnail.component.html @@ -1,4 +1,4 @@ -
+
@@ -6,16 +6,14 @@
- - - -
-
-
- {{ placeholder | translate }} -
+ + +
+
+
+ {{ placeholder | translate }}
- +
diff --git a/src/app/thumbnail/thumbnail.component.spec.ts b/src/app/thumbnail/thumbnail.component.spec.ts index ebecb5e075..2a25c9a318 100644 --- a/src/app/thumbnail/thumbnail.component.spec.ts +++ b/src/app/thumbnail/thumbnail.component.spec.ts @@ -67,31 +67,31 @@ describe('ThumbnailComponent', () => { describe('loading', () => { it('should start out with isLoading$ true', () => { - expect(comp.isLoading$.getValue()).toBeTrue(); + expect(comp.isLoading).toBeTrue(); }); it('should set isLoading$ to false once an image is successfully loaded', () => { comp.setSrc('http://bit.stream'); fixture.debugElement.query(By.css('img.thumbnail-content')).triggerEventHandler('load', new Event('load')); - expect(comp.isLoading$.getValue()).toBeFalse(); + expect(comp.isLoading).toBeFalse(); }); it('should set isLoading$ to false once the src is set to null', () => { comp.setSrc(null); - expect(comp.isLoading$.getValue()).toBeFalse(); + expect(comp.isLoading).toBeFalse(); }); it('should show a loading animation while isLoading$ is true', () => { expect(de.query(By.css('ds-themed-loading'))).toBeTruthy(); - comp.isLoading$.next(false); + comp.isLoading = false; fixture.detectChanges(); expect(fixture.debugElement.query(By.css('ds-themed-loading'))).toBeFalsy(); }); describe('with a thumbnail image', () => { beforeEach(() => { - comp.src$.next('https://bit.stream'); + comp.src = 'https://bit.stream'; fixture.detectChanges(); }); @@ -100,7 +100,7 @@ describe('ThumbnailComponent', () => { expect(img).toBeTruthy(); expect(img.classes['d-none']).toBeTrue(); - comp.isLoading$.next(false); + comp.isLoading = false; fixture.detectChanges(); img = fixture.debugElement.query(By.css('img.thumbnail-content')); expect(img).toBeTruthy(); @@ -111,14 +111,14 @@ describe('ThumbnailComponent', () => { describe('without a thumbnail image', () => { beforeEach(() => { - comp.src$.next(null); + comp.src = null; fixture.detectChanges(); }); it('should only show the HTML placeholder once done loading', () => { expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeFalsy(); - comp.isLoading$.next(false); + comp.isLoading = false; fixture.detectChanges(); expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeTruthy(); }); @@ -214,14 +214,14 @@ describe('ThumbnailComponent', () => { describe('fallback', () => { describe('if there is a default image', () => { it('should display the default image', () => { - comp.src$.next('http://bit.stream'); + comp.src = 'http://bit.stream'; comp.defaultImage = 'http://default.img'; comp.errorHandler(); - expect(comp.src$.getValue()).toBe(comp.defaultImage); + expect(comp.src).toBe(comp.defaultImage); }); it('should include the alt text', () => { - comp.src$.next('http://bit.stream'); + comp.src = 'http://bit.stream'; comp.defaultImage = 'http://default.img'; comp.errorHandler(); @@ -233,10 +233,10 @@ describe('ThumbnailComponent', () => { describe('if there is no default image', () => { it('should display the HTML placeholder', () => { - comp.src$.next('http://default.img'); + comp.src = 'http://default.img'; comp.defaultImage = null; comp.errorHandler(); - expect(comp.src$.getValue()).toBe(null); + expect(comp.src).toBe(null); fixture.detectChanges(); const placeholder = fixture.debugElement.query(By.css('div.thumbnail-placeholder')).nativeElement; @@ -328,7 +328,7 @@ describe('ThumbnailComponent', () => { it('should show the default image', () => { comp.defaultImage = 'default/image.jpg'; comp.ngOnChanges({}); - expect(comp.src$.getValue()).toBe('default/image.jpg'); + expect(comp.src).toBe('default/image.jpg'); }); }); }); diff --git a/src/app/thumbnail/thumbnail.component.ts b/src/app/thumbnail/thumbnail.component.ts index 7f6531cc5e..0827a33e77 100644 --- a/src/app/thumbnail/thumbnail.component.ts +++ b/src/app/thumbnail/thumbnail.component.ts @@ -2,7 +2,7 @@ import { Component, Input, OnChanges, SimpleChanges } from '@angular/core'; import { Bitstream } from '../core/shared/bitstream.model'; import { hasNoValue, hasValue } from '../shared/empty.util'; import { RemoteData } from '../core/data/remote-data'; -import { BehaviorSubject, of as observableOf } from 'rxjs'; +import { of as observableOf } from 'rxjs'; import { switchMap } from 'rxjs/operators'; import { FeatureID } from '../core/data/feature-authorization/feature-id'; import { AuthorizationDataService } from '../core/data/feature-authorization/authorization-data.service'; @@ -34,7 +34,7 @@ export class ThumbnailComponent implements OnChanges { /** * The src attribute used in the template to render the image. */ - src$ = new BehaviorSubject(undefined); + src: string = undefined; retriedWithToken = false; @@ -57,7 +57,7 @@ export class ThumbnailComponent implements OnChanges { * Whether the thumbnail is currently loading * Start out as true to avoid flashing the alt text while a thumbnail is being loaded. */ - isLoading$ = new BehaviorSubject(true); + isLoading = true; constructor( protected auth: AuthService, @@ -110,7 +110,7 @@ export class ThumbnailComponent implements OnChanges { * Otherwise, fall back to the default image or a HTML placeholder */ errorHandler() { - const src = this.src$.getValue(); + const src = this.src; const thumbnail = this.bitstream; const thumbnailSrc = thumbnail?._links?.content?.href; @@ -162,9 +162,9 @@ export class ThumbnailComponent implements OnChanges { * @param src */ setSrc(src: string): void { - this.src$.next(src); + this.src = src; if (src === null) { - this.isLoading$.next(false); + this.isLoading = false; } } @@ -172,6 +172,6 @@ export class ThumbnailComponent implements OnChanges { * Stop the loading animation once the thumbnail is successfully loaded */ successHandler() { - this.isLoading$.next(false); + this.isLoading = false; } } From 11f251755b29de51ab377b824c58d96c61983144 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Fri, 25 Apr 2025 14:55:52 +0200 Subject: [PATCH 2/5] fix issue where thumnails of embargoed bitstreams wouldn't show up for users with access rights --- src/app/thumbnail/thumbnail.component.html | 10 ++--- src/app/thumbnail/thumbnail.component.spec.ts | 28 ++++++------ src/app/thumbnail/thumbnail.component.ts | 45 ++++++++++++------- 3 files changed, 47 insertions(+), 36 deletions(-) diff --git a/src/app/thumbnail/thumbnail.component.html b/src/app/thumbnail/thumbnail.component.html index 049a47e1b0..bd161ea748 100644 --- a/src/app/thumbnail/thumbnail.component.html +++ b/src/app/thumbnail/thumbnail.component.html @@ -1,15 +1,15 @@
-
+
- - -
+ + +
{{ placeholder | translate }} diff --git a/src/app/thumbnail/thumbnail.component.spec.ts b/src/app/thumbnail/thumbnail.component.spec.ts index 2a25c9a318..ebecb5e075 100644 --- a/src/app/thumbnail/thumbnail.component.spec.ts +++ b/src/app/thumbnail/thumbnail.component.spec.ts @@ -67,31 +67,31 @@ describe('ThumbnailComponent', () => { describe('loading', () => { it('should start out with isLoading$ true', () => { - expect(comp.isLoading).toBeTrue(); + expect(comp.isLoading$.getValue()).toBeTrue(); }); it('should set isLoading$ to false once an image is successfully loaded', () => { comp.setSrc('http://bit.stream'); fixture.debugElement.query(By.css('img.thumbnail-content')).triggerEventHandler('load', new Event('load')); - expect(comp.isLoading).toBeFalse(); + expect(comp.isLoading$.getValue()).toBeFalse(); }); it('should set isLoading$ to false once the src is set to null', () => { comp.setSrc(null); - expect(comp.isLoading).toBeFalse(); + expect(comp.isLoading$.getValue()).toBeFalse(); }); it('should show a loading animation while isLoading$ is true', () => { expect(de.query(By.css('ds-themed-loading'))).toBeTruthy(); - comp.isLoading = false; + comp.isLoading$.next(false); fixture.detectChanges(); expect(fixture.debugElement.query(By.css('ds-themed-loading'))).toBeFalsy(); }); describe('with a thumbnail image', () => { beforeEach(() => { - comp.src = 'https://bit.stream'; + comp.src$.next('https://bit.stream'); fixture.detectChanges(); }); @@ -100,7 +100,7 @@ describe('ThumbnailComponent', () => { expect(img).toBeTruthy(); expect(img.classes['d-none']).toBeTrue(); - comp.isLoading = false; + comp.isLoading$.next(false); fixture.detectChanges(); img = fixture.debugElement.query(By.css('img.thumbnail-content')); expect(img).toBeTruthy(); @@ -111,14 +111,14 @@ describe('ThumbnailComponent', () => { describe('without a thumbnail image', () => { beforeEach(() => { - comp.src = null; + comp.src$.next(null); fixture.detectChanges(); }); it('should only show the HTML placeholder once done loading', () => { expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeFalsy(); - comp.isLoading = false; + comp.isLoading$.next(false); fixture.detectChanges(); expect(fixture.debugElement.query(By.css('div.thumbnail-placeholder'))).toBeTruthy(); }); @@ -214,14 +214,14 @@ describe('ThumbnailComponent', () => { describe('fallback', () => { describe('if there is a default image', () => { it('should display the default image', () => { - comp.src = 'http://bit.stream'; + comp.src$.next('http://bit.stream'); comp.defaultImage = 'http://default.img'; comp.errorHandler(); - expect(comp.src).toBe(comp.defaultImage); + expect(comp.src$.getValue()).toBe(comp.defaultImage); }); it('should include the alt text', () => { - comp.src = 'http://bit.stream'; + comp.src$.next('http://bit.stream'); comp.defaultImage = 'http://default.img'; comp.errorHandler(); @@ -233,10 +233,10 @@ describe('ThumbnailComponent', () => { describe('if there is no default image', () => { it('should display the HTML placeholder', () => { - comp.src = 'http://default.img'; + comp.src$.next('http://default.img'); comp.defaultImage = null; comp.errorHandler(); - expect(comp.src).toBe(null); + expect(comp.src$.getValue()).toBe(null); fixture.detectChanges(); const placeholder = fixture.debugElement.query(By.css('div.thumbnail-placeholder')).nativeElement; @@ -328,7 +328,7 @@ describe('ThumbnailComponent', () => { it('should show the default image', () => { comp.defaultImage = 'default/image.jpg'; comp.ngOnChanges({}); - expect(comp.src).toBe('default/image.jpg'); + expect(comp.src$.getValue()).toBe('default/image.jpg'); }); }); }); diff --git a/src/app/thumbnail/thumbnail.component.ts b/src/app/thumbnail/thumbnail.component.ts index 0827a33e77..842ead951f 100644 --- a/src/app/thumbnail/thumbnail.component.ts +++ b/src/app/thumbnail/thumbnail.component.ts @@ -1,13 +1,14 @@ -import { Component, Input, OnChanges, SimpleChanges } from '@angular/core'; +import { Component, Inject, Input, OnChanges, PLATFORM_ID, SimpleChanges } from '@angular/core'; import { Bitstream } from '../core/shared/bitstream.model'; import { hasNoValue, hasValue } from '../shared/empty.util'; import { RemoteData } from '../core/data/remote-data'; -import { of as observableOf } from 'rxjs'; +import { of as observableOf, BehaviorSubject } from 'rxjs'; import { switchMap } from 'rxjs/operators'; import { FeatureID } from '../core/data/feature-authorization/feature-id'; import { AuthorizationDataService } from '../core/data/feature-authorization/authorization-data.service'; import { AuthService } from '../core/auth/auth.service'; import { FileService } from '../core/shared/file.service'; +import { isPlatformBrowser } from '@angular/common'; /** * This component renders a given Bitstream as a thumbnail. @@ -34,7 +35,7 @@ export class ThumbnailComponent implements OnChanges { /** * The src attribute used in the template to render the image. */ - src: string = undefined; + src$: BehaviorSubject = new BehaviorSubject(undefined); retriedWithToken = false; @@ -57,9 +58,10 @@ export class ThumbnailComponent implements OnChanges { * Whether the thumbnail is currently loading * Start out as true to avoid flashing the alt text while a thumbnail is being loaded. */ - isLoading = true; + isLoading$: BehaviorSubject = new BehaviorSubject(true); constructor( + @Inject(PLATFORM_ID) private platformID: any, protected auth: AuthService, protected authorizationService: AuthorizationDataService, protected fileService: FileService, @@ -71,16 +73,25 @@ export class ThumbnailComponent implements OnChanges { * Use a default image if no actual image is available. */ ngOnChanges(changes: SimpleChanges): void { - if (hasNoValue(this.thumbnail)) { - this.setSrc(this.defaultImage); - return; - } + if (isPlatformBrowser(this.platformID)) { + // every time the inputs change we need to start the loading animation again, as it's possible + // that thumbnail is first set to null when the parent component initializes and then set to + // the actual value + if (this.isLoading$.getValue() === false) { + this.isLoading$.next(true); + } - const src = this.contentHref; - if (hasValue(src)) { - this.setSrc(src); - } else { - this.setSrc(this.defaultImage); + if (hasNoValue(this.thumbnail)) { + this.setSrc(this.defaultImage); + return; + } + + const src = this.contentHref; + if (hasValue(src)) { + this.setSrc(src); + } else { + this.setSrc(this.defaultImage); + } } } @@ -110,7 +121,7 @@ export class ThumbnailComponent implements OnChanges { * Otherwise, fall back to the default image or a HTML placeholder */ errorHandler() { - const src = this.src; + const src = this.src$.getValue(); const thumbnail = this.bitstream; const thumbnailSrc = thumbnail?._links?.content?.href; @@ -162,9 +173,9 @@ export class ThumbnailComponent implements OnChanges { * @param src */ setSrc(src: string): void { - this.src = src; + this.src$.next(src); if (src === null) { - this.isLoading = false; + this.isLoading$.next(false); } } @@ -172,6 +183,6 @@ export class ThumbnailComponent implements OnChanges { * Stop the loading animation once the thumbnail is successfully loaded */ successHandler() { - this.isLoading = false; + this.isLoading$.next(false); } } From e40a44c5ac1f2583d8338a2ddcb56fee2158acc5 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 6 May 2025 14:38:45 +0200 Subject: [PATCH 3/5] fix issue where thumbnail would sometimes keep loading indefinitely --- src/app/thumbnail/thumbnail.component.ts | 26 +++++++++++++++--------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/app/thumbnail/thumbnail.component.ts b/src/app/thumbnail/thumbnail.component.ts index 842ead951f..d8dea1dadd 100644 --- a/src/app/thumbnail/thumbnail.component.ts +++ b/src/app/thumbnail/thumbnail.component.ts @@ -74,13 +74,6 @@ export class ThumbnailComponent implements OnChanges { */ ngOnChanges(changes: SimpleChanges): void { if (isPlatformBrowser(this.platformID)) { - // every time the inputs change we need to start the loading animation again, as it's possible - // that thumbnail is first set to null when the parent component initializes and then set to - // the actual value - if (this.isLoading$.getValue() === false) { - this.isLoading$.next(true); - } - if (hasNoValue(this.thumbnail)) { this.setSrc(this.defaultImage); return; @@ -173,9 +166,22 @@ export class ThumbnailComponent implements OnChanges { * @param src */ setSrc(src: string): void { - this.src$.next(src); - if (src === null) { - this.isLoading$.next(false); + // only update the src if it has changed (the parent component may fire the same one multiple times + if (this.src$.getValue() !== src) { + // every time the src changes we need to start the loading animation again, as it's possible + // that it is first set to null when the parent component initializes and then set to + // the actual value + // + // isLoading$ will be set to false by the error or success handler afterwards, except in the + // case where src is null, then we have to set it manually here (because those handlers won't + // trigger) + if (this.isLoading$.getValue() === false) { + this.isLoading$.next(true); + } + this.src$.next(src); + if (src === null) { + this.isLoading$.next(false); + } } } From 70d1e499c10bdfbdcc450abd58dd2a59e954a156 Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Tue, 6 May 2025 14:58:53 +0200 Subject: [PATCH 4/5] don't show the loading animation when src is set to null --- src/app/thumbnail/thumbnail.component.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/app/thumbnail/thumbnail.component.ts b/src/app/thumbnail/thumbnail.component.ts index d8dea1dadd..5f8927f3a2 100644 --- a/src/app/thumbnail/thumbnail.component.ts +++ b/src/app/thumbnail/thumbnail.component.ts @@ -175,11 +175,11 @@ export class ThumbnailComponent implements OnChanges { // isLoading$ will be set to false by the error or success handler afterwards, except in the // case where src is null, then we have to set it manually here (because those handlers won't // trigger) - if (this.isLoading$.getValue() === false) { + if (src !== null && this.isLoading$.getValue() === false) { this.isLoading$.next(true); } this.src$.next(src); - if (src === null) { + if (src === null && this.isLoading$.getValue() === true) { this.isLoading$.next(false); } } From 283b345cdc8a034593404c226aebf7f5994b0f5f Mon Sep 17 00:00:00 2001 From: Art Lowel Date: Thu, 8 May 2025 11:26:14 +0200 Subject: [PATCH 5/5] always use thumbnail component for files not supported by the media viewer, and switch to themed version of thumbnail component --- .../item-page/media-viewer/media-viewer.component.html | 9 ++------- .../media-viewer/media-viewer.component.spec.ts | 4 ++-- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/app/item-page/media-viewer/media-viewer.component.html b/src/app/item-page/media-viewer/media-viewer.component.html index c8a02e039c..eb491c5193 100644 --- a/src/app/item-page/media-viewer/media-viewer.component.html +++ b/src/app/item-page/media-viewer/media-viewer.component.html @@ -16,12 +16,7 @@
- - - + + diff --git a/src/app/item-page/media-viewer/media-viewer.component.spec.ts b/src/app/item-page/media-viewer/media-viewer.component.spec.ts index 0c170ac8cf..6de0d82595 100644 --- a/src/app/item-page/media-viewer/media-viewer.component.spec.ts +++ b/src/app/item-page/media-viewer/media-viewer.component.spec.ts @@ -139,9 +139,9 @@ describe('MediaViewerComponent', () => { expect(mediaItem.thumbnail).toBe(null); }); - it('should display a default, thumbnail', () => { + it('should display a default thumbnail', () => { const defaultThumbnail = fixture.debugElement.query( - By.css('ds-themed-media-viewer-image') + By.css('ds-themed-thumbnail') ); expect(defaultThumbnail.nativeElement).toBeDefined(); });