From 010b2f9693da1e6905951118f35aefc2a7f6a3fa Mon Sep 17 00:00:00 2001 From: Andreas Awouters Date: Wed, 11 Dec 2024 09:35:01 +0100 Subject: [PATCH] 119602: Improve types & docs --- .../accessibility-settings.service.spec.ts | 36 ++++++----------- .../accessibility-settings.service.ts | 40 +++++++++++-------- .../accessibility-settings.component.html | 4 +- .../accessibility-settings.component.ts | 3 -- .../shared/live-region/live-region.service.ts | 4 +- .../notifications-board.component.ts | 5 +-- 6 files changed, 41 insertions(+), 51 deletions(-) diff --git a/src/app/accessibility/accessibility-settings.service.spec.ts b/src/app/accessibility/accessibility-settings.service.spec.ts index 5344c31d452..3d58c02d109 100644 --- a/src/app/accessibility/accessibility-settings.service.spec.ts +++ b/src/app/accessibility/accessibility-settings.service.spec.ts @@ -1,6 +1,5 @@ import { AccessibilitySettingsService, - AccessibilitySetting, AccessibilitySettings, ACCESSIBILITY_SETTINGS_METADATA_KEY, ACCESSIBILITY_COOKIE @@ -41,17 +40,6 @@ describe('accessibilitySettingsService', () => { ); }); - describe('getALlAccessibilitySettingsKeys', () => { - it('should return an array containing all accessibility setting names', () => { - const settingNames: AccessibilitySetting[] = [ - AccessibilitySetting.NotificationTimeOut, - AccessibilitySetting.LiveRegionTimeOut, - ]; - - expect(service.getAllAccessibilitySettingKeys()).toEqual(settingNames); - }); - }); - describe('get', () => { it('should return the setting if it is set', () => { const settings: AccessibilitySettings = { @@ -60,7 +48,7 @@ describe('accessibilitySettingsService', () => { service.getAll = jasmine.createSpy('getAll').and.returnValue(of(settings)); - service.get(AccessibilitySetting.NotificationTimeOut, 'default').subscribe(value => + service.get('notificationTimeOut', 'default').subscribe(value => expect(value).toEqual('1000') ); }); @@ -72,7 +60,7 @@ describe('accessibilitySettingsService', () => { service.getAll = jasmine.createSpy('getAll').and.returnValue(of(settings)); - service.get(AccessibilitySetting.LiveRegionTimeOut, 'default').subscribe(value => + service.get('liveRegionTimeOut', 'default').subscribe(value => expect(value).toEqual('default') ); }); @@ -82,7 +70,7 @@ describe('accessibilitySettingsService', () => { it('should return the setting as number if the value for the setting can be parsed to a number', () => { service.get = jasmine.createSpy('get').and.returnValue(of('1000')); - service.getAsNumber(AccessibilitySetting.NotificationTimeOut).subscribe(value => + service.getAsNumber('notificationTimeOut').subscribe(value => expect(value).toEqual(1000) ); }); @@ -90,7 +78,7 @@ describe('accessibilitySettingsService', () => { it('should return the default value if no value is set for the setting', () => { service.get = jasmine.createSpy('get').and.returnValue(of(null)); - service.getAsNumber(AccessibilitySetting.NotificationTimeOut, 123).subscribe(value => + service.getAsNumber('notificationTimeOut', 123).subscribe(value => expect(value).toEqual(123) ); }); @@ -98,7 +86,7 @@ describe('accessibilitySettingsService', () => { it('should return the default value if the value for the setting can not be parsed to a number', () => { service.get = jasmine.createSpy('get').and.returnValue(of('text')); - service.getAsNumber(AccessibilitySetting.NotificationTimeOut, 123).subscribe(value => + service.getAsNumber('notificationTimeOut', 123).subscribe(value => expect(value).toEqual(123) ); }); @@ -176,7 +164,7 @@ describe('accessibilitySettingsService', () => { it('should correctly update the chosen setting', () => { service.updateSettings = jasmine.createSpy('updateSettings'); - service.set(AccessibilitySetting.LiveRegionTimeOut, '500'); + service.set('liveRegionTimeOut', '500'); expect(service.updateSettings).toHaveBeenCalledWith({ liveRegionTimeOut: '500' }); }); }); @@ -307,7 +295,7 @@ describe('accessibilitySettingsService', () => { }); it('should set the settings in metadata', () => { - service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe(); + service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe(); expect(ePerson.setMetadata).toHaveBeenCalled(); }); @@ -318,19 +306,19 @@ describe('accessibilitySettingsService', () => { }); it('should create a patch with the metadata changes', () => { - service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe(); + service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe(); expect(ePersonService.createPatchFromCache).toHaveBeenCalled(); }); it('should send the patch request', () => { - service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }).subscribe(); + service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }).subscribe(); expect(ePersonService.patch).toHaveBeenCalled(); }); it('should emit true when the update succeeded', fakeAsync(() => { ePersonService.patch = jasmine.createSpy().and.returnValue(createSuccessfulRemoteDataObject$({})); - service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }) + service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }) .subscribe(value => { expect(value).toBeTrue(); }); @@ -341,7 +329,7 @@ describe('accessibilitySettingsService', () => { it('should emit false when the update failed', fakeAsync(() => { ePersonService.patch = jasmine.createSpy().and.returnValue(createFailedRemoteDataObject$()); - service.setSettingsInMetadata(ePerson, { [AccessibilitySetting.LiveRegionTimeOut]: '500' }) + service.setSettingsInMetadata(ePerson, { ['liveRegionTimeOut']: '500' }) .subscribe(value => { expect(value).toBeFalse(); }); @@ -357,7 +345,7 @@ describe('accessibilitySettingsService', () => { }); it('should store the settings in a cookie', () => { - service.setSettingsInCookie({ [AccessibilitySetting.LiveRegionTimeOut]: '500' }); + service.setSettingsInCookie({ ['liveRegionTimeOut']: '500' }); expect(cookieService.set).toHaveBeenCalled(); }); diff --git a/src/app/accessibility/accessibility-settings.service.ts b/src/app/accessibility/accessibility-settings.service.ts index f2151675391..e9e41954c7c 100644 --- a/src/app/accessibility/accessibility-settings.service.ts +++ b/src/app/accessibility/accessibility-settings.service.ts @@ -22,19 +22,21 @@ export const ACCESSIBILITY_COOKIE = 'dsAccessibilityCookie'; export const ACCESSIBILITY_SETTINGS_METADATA_KEY = 'dspace.accessibility.settings'; /** - * Enum containing all possible accessibility settings. - * When adding new settings, make sure to add the new setting to the accessibility-settings component. + * Type containing all possible accessibility settings. + * When adding new settings, make sure to add the new setting to the accessibility-settings component form. * The converter methods to convert from stored format to form format (and vice-versa) need to be updated as well. */ -export enum AccessibilitySetting { - NotificationTimeOut = 'notificationTimeOut', - LiveRegionTimeOut = 'liveRegionTimeOut', -} +export type AccessibilitySetting = 'notificationTimeOut' | 'liveRegionTimeOut'; /** - * Type representing an object that contains accessibility settings values. + * Type representing an object that contains accessibility settings values for all accessibility settings. */ -export type AccessibilitySettings = { [key in AccessibilitySetting]?: string }; +export type FullAccessibilitySettings = { [key in AccessibilitySetting]: string }; + +/** + * Type representing an object that contains accessibility settings values for some accessibility settings. + */ +export type AccessibilitySettings = Partial; /** * The accessibility settings object format used by the accessibility-settings component form. @@ -63,10 +65,6 @@ export class AccessibilitySettingsService { ) { } - getAllAccessibilitySettingKeys(): AccessibilitySetting[] { - return Object.entries(AccessibilitySetting).map(([_, val]) => val); - } - /** * Get the stored value for the provided {@link AccessibilitySetting}. If the value does not exist or if it is empty, * the provided defaultValue is emitted instead. @@ -238,9 +236,9 @@ export class AccessibilitySettingsService { */ getPlaceholder(setting: AccessibilitySetting): string { switch (setting) { - case AccessibilitySetting.NotificationTimeOut: + case 'notificationTimeOut': return millisecondsToSeconds(environment.notifications.timeOut.toString()); - case AccessibilitySetting.LiveRegionTimeOut: + case 'liveRegionTimeOut': return millisecondsToSeconds(environment.liveRegion.messageTimeOutDurationMs.toString()); default: return ''; @@ -250,11 +248,11 @@ export class AccessibilitySettingsService { /** * Convert values in the provided accessibility settings object to values ready to be stored. */ - convertFormValuesToStoredValues(settings: AccessibilitySettingsFormValues): AccessibilitySettings { + convertFormValuesToStoredValues(settings: AccessibilitySettingsFormValues): FullAccessibilitySettings { return { - 'notificationTimeOut': settings.disableNotificationTimeOut ? '0' + notificationTimeOut: settings.disableNotificationTimeOut ? '0' : secondsToMilliseconds(settings.notificationTimeOut), - 'liveRegionTimeOut': secondsToMilliseconds(settings.liveRegionTimeOut), + liveRegionTimeOut: secondsToMilliseconds(settings.liveRegionTimeOut), }; } @@ -271,6 +269,10 @@ export class AccessibilitySettingsService { } +/** + * Converts a string representing seconds to a string representing milliseconds + * Returns null if the input could not be parsed to a float + */ function secondsToMilliseconds(secondsStr: string): string { const seconds = parseFloat(secondsStr); if (isNaN(seconds)) { @@ -280,6 +282,10 @@ function secondsToMilliseconds(secondsStr: string): string { } } +/** + * Converts a string representing milliseconds to a string representing seconds + * Returns null if the input could not be parsed to a float + */ function millisecondsToSeconds(millisecondsStr: string): string { const milliseconds = parseFloat(millisecondsStr); if (isNaN(milliseconds)) { diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.html b/src/app/info/accessibility-settings/accessibility-settings.component.html index d247b12f949..5a5493f0bea 100644 --- a/src/app/info/accessibility-settings/accessibility-settings.component.html +++ b/src/app/info/accessibility-settings/accessibility-settings.component.html @@ -30,7 +30,7 @@

{{ 'info.accessibility-settings.title' | translate }}

@@ -56,7 +56,7 @@

{{ 'info.accessibility-settings.title' | translate }}

diff --git a/src/app/info/accessibility-settings/accessibility-settings.component.ts b/src/app/info/accessibility-settings/accessibility-settings.component.ts index cd417393d44..d537c363c3f 100644 --- a/src/app/info/accessibility-settings/accessibility-settings.component.ts +++ b/src/app/info/accessibility-settings/accessibility-settings.component.ts @@ -18,9 +18,6 @@ import { TranslateService } from '@ngx-translate/core'; }) export class AccessibilitySettingsComponent implements OnInit { - // Re-export for use in template - protected readonly AccessibilitySetting = AccessibilitySetting; - protected formValues: AccessibilitySettingsFormValues; constructor( diff --git a/src/app/shared/live-region/live-region.service.ts b/src/app/shared/live-region/live-region.service.ts index 7a7b99fa6e7..37f58a2795d 100644 --- a/src/app/shared/live-region/live-region.service.ts +++ b/src/app/shared/live-region/live-region.service.ts @@ -2,7 +2,7 @@ import { Injectable } from '@angular/core'; import { BehaviorSubject, map, Observable, switchMap, take, timer } from 'rxjs'; import { environment } from '../../../environments/environment'; import { UUIDService } from '../../core/shared/uuid.service'; -import { AccessibilitySettingsService, AccessibilitySetting } from '../../accessibility/accessibility-settings.service'; +import { AccessibilitySettingsService } from '../../accessibility/accessibility-settings.service'; export const MIN_MESSAGE_DURATION = 200; @@ -130,7 +130,7 @@ export class LiveRegionService { */ getConfiguredMessageTimeOutMs(): Observable { return this.accessibilitySettingsService.getAsNumber( - AccessibilitySetting.LiveRegionTimeOut, + 'liveRegionTimeOut', this.getMessageTimeOutMs(), ).pipe(map(timeOut => Math.max(timeOut, MIN_MESSAGE_DURATION))); } diff --git a/src/app/shared/notifications/notifications-board/notifications-board.component.ts b/src/app/shared/notifications/notifications-board/notifications-board.component.ts index 8439a1af16d..96319416268 100644 --- a/src/app/shared/notifications/notifications-board/notifications-board.component.ts +++ b/src/app/shared/notifications/notifications-board/notifications-board.component.ts @@ -18,8 +18,7 @@ import { INotification } from '../models/notification.model'; import { NotificationsState } from '../notifications.reducers'; import { INotificationBoardOptions } from '../../../../config/notifications-config.interfaces'; import { - AccessibilitySettingsService, - AccessibilitySetting + AccessibilitySettingsService } from '../../../accessibility/accessibility-settings.service'; import cloneDeep from 'lodash/cloneDeep'; import differenceWith from 'lodash/differenceWith'; @@ -98,7 +97,7 @@ export class NotificationsBoardComponent implements OnInit, OnDestroy { // It would be a bit better to handle the retrieval of configured settings in the NotificationsService. // Due to circular dependencies this is difficult to implement. - this.accessibilitySettingsService.getAsNumber(AccessibilitySetting.NotificationTimeOut, item.options.timeOut) + this.accessibilitySettingsService.getAsNumber('notificationTimeOut', item.options.timeOut) .pipe(take(1)).subscribe(timeOut => { if (timeOut < 0) { timeOut = 0;