refactor(web): cleanup notification components (#8150)

* refactor(web): cleanup notification components * use counter for ID
immich-app · Mar 21, 2024 · 7930493 · 7930493
1 parent 382b639
commit 7930493
Show file tree

Hide file tree

Showing 5 changed files with 66 additions and 111 deletions.
diff --git a/web/src/lib/components/shared-components/notification/__tests__/notification-card.spec.ts b/web/src/lib/components/shared-components/notification/__tests__/notification-card.spec.ts
@@ -10,7 +10,7 @@ describe('NotificationCard component', () => {
     vi.spyOn(window, 'clearTimeout');
 
     sut = render(NotificationCard, {
-      notificationInfo: {
+      notification: {
         id: 1234,
         message: 'Notification message',
         timeout: 1000,
@@ -25,7 +25,7 @@ describe('NotificationCard component', () => {
 
   it('shows message and title', () => {
     sut = render(NotificationCard, {
-      notificationInfo: {
+      notification: {
         id: 1234,
         message: 'Notification message',
         timeout: 1000,

diff --git a/web/src/lib/components/shared-components/notification/__tests__/notification-list.spec.ts b/web/src/lib/components/shared-components/notification/__tests__/notification-list.spec.ts
@@ -12,11 +12,14 @@ describe('NotificationList component', () => {
   const sut: RenderResult<NotificationList> = render(NotificationList);
 
   beforeAll(() => {
-    vi.useFakeTimers();
+    // https://testing-library.com/docs/svelte-testing-library/faq#why-arent-transition-events-running
+    vi.stubGlobal('requestAnimationFrame', (fn: FrameRequestCallback) => {
+      setTimeout(() => fn(Date.now()), 16);
+    });
   });
 
   afterAll(() => {
-    vi.useRealTimers();
+    vi.unstubAllGlobals();
   });
 
   it('shows a notification when added and closes it automatically after the delay timeout', async () => {
@@ -25,18 +28,14 @@ describe('NotificationList component', () => {
     notificationController.show({
       message: 'Notification',
       type: NotificationType.Info,
-      timeout: 3000,
+      timeout: 1,
     });
 
     await waitFor(() => expect(_getNotificationListElement(sut)).toBeInTheDocument());
+    await waitFor(() => expect(_getNotificationListElement(sut)?.children).toHaveLength(1));
+    expect(get(notificationController.notificationList)).toHaveLength(1);
 
-    expect(_getNotificationListElement(sut)?.children).toHaveLength(1);
-
-    vi.advanceTimersByTime(4000);
-    // due to some weirdness in svelte (or testing-library) need to check if it has been removed from the store to make sure it works.
+    await waitFor(() => expect(_getNotificationListElement(sut)).not.toBeInTheDocument());
     expect(get(notificationController.notificationList)).toHaveLength(0);
-
-    // TODO: investigate why this element is not removed from the DOM even notification list is in fact 0.
-    // await waitFor(() => expect(_getNotificationListElement(sut)).not.toBeInTheDocument());
   });
 });
diff --git a/web/src/lib/components/shared-components/notification/notification-card.svelte b/web/src/lib/components/shared-components/notification/notification-card.svelte
@@ -2,77 +2,47 @@
   import { fade } from 'svelte/transition';
   import Icon from '$lib/components/elements/icon.svelte';
   import {
-    ImmichNotification,
+    type Notification,
     notificationController,
     NotificationType,
   } from '$lib/components/shared-components/notification/notification';
   import { onMount } from 'svelte';
   import { mdiCloseCircleOutline, mdiInformationOutline, mdiWindowClose } from '@mdi/js';
 
-  export let notificationInfo: ImmichNotification;
+  export let notification: Notification;
 
-  let infoPrimaryColor = '#4250AF';
-  let errorPrimaryColor = '#E64132';
-  let warningPrimaryColor = '#D08613';
+  $: icon = notification.type === NotificationType.Error ? mdiCloseCircleOutline : mdiInformationOutline;
 
-  $: icon = notificationInfo.type === NotificationType.Error ? mdiCloseCircleOutline : mdiInformationOutline;
-
-  $: backgroundColor = () => {
-    if (notificationInfo.type === NotificationType.Info) {
-      return '#E0E2F0';
-    }
-
-    if (notificationInfo.type === NotificationType.Error) {
-      return '#FBE8E6';
-    }
-
-    if (notificationInfo.type === NotificationType.Warning) {
-      return '#FFF6DC';
-    }
+  const backgroundColor: Record<NotificationType, string> = {
+    [NotificationType.Info]: '#E0E2F0',
+    [NotificationType.Error]: '#FBE8E6',
+    [NotificationType.Warning]: '#FFF6DC',
   };
 
-  $: borderStyle = () => {
-    if (notificationInfo.type === NotificationType.Info) {
-      return '1px solid #D8DDFF';
-    }
-
-    if (notificationInfo.type === NotificationType.Error) {
-      return '1px solid #F0E8E7';
-    }
-
-    if (notificationInfo.type === NotificationType.Warning) {
-      return '1px solid #FFE6A5';
-    }
+  const borderColor: Record<NotificationType, string> = {
+    [NotificationType.Info]: '#D8DDFF',
+    [NotificationType.Error]: '#F0E8E7',
+    [NotificationType.Warning]: '#FFE6A5',
   };
 
-  $: primaryColor = () => {
-    if (notificationInfo.type === NotificationType.Info) {
-      return infoPrimaryColor;
-    }
-
-    if (notificationInfo.type === NotificationType.Error) {
-      return errorPrimaryColor;
-    }
-
-    if (notificationInfo.type === NotificationType.Warning) {
-      return warningPrimaryColor;
-    }
+  const primaryColor: Record<NotificationType, string> = {
+    [NotificationType.Info]: '#4250AF',
+    [NotificationType.Error]: '#E64132',
+    [NotificationType.Warning]: '#D08613',
   };
 
-  let removeNotificationTimeout: ReturnType<typeof setTimeout> | undefined;
-
   onMount(() => {
-    removeNotificationTimeout = setTimeout(discard, notificationInfo.timeout);
-    return () => clearTimeout(removeNotificationTimeout);
+    const timeoutId = setTimeout(discard, notification.timeout);
+    return () => clearTimeout(timeoutId);
   });
 
   const discard = () => {
-    notificationController.removeNotificationById(notificationInfo.id);
+    notificationController.removeNotificationById(notification.id);
   };
 
   const handleClick = () => {
-    const action = notificationInfo.action;
-    if (action.type == 'discard') {
+    const action = notification.action;
+    if (action.type === 'discard') {
       discard();
     } else if (action.type == 'link') {
       window.open(action.target);
@@ -83,17 +53,17 @@
 <!-- svelte-ignore a11y-no-static-element-interactions -->
 <div
   transition:fade={{ duration: 250 }}
-  style:background-color={backgroundColor()}
-  style:border={borderStyle()}
-  class="z-[999999] mb-4 min-h-[80px] w-[300px] rounded-2xl p-4 shadow-md hover:cursor-pointer"
+  style:background-color={backgroundColor[notification.type]}
+  style:border-color={borderColor[notification.type]}
+  class="border z-[999999] mb-4 min-h-[80px] w-[300px] rounded-2xl p-4 shadow-md hover:cursor-pointer"
   on:click={handleClick}
   on:keydown={handleClick}
 >
   <div class="flex justify-between">
     <div class="flex place-items-center gap-2">
-      <Icon path={icon} color={primaryColor()} size="20" />
-      <h2 style:color={primaryColor()} class="font-medium" data-testid="title">
-        {notificationInfo.type.toString()}
+      <Icon path={icon} color={primaryColor[notification.type]} size="20" />
+      <h2 style:color={primaryColor[notification.type]} class="font-medium" data-testid="title">
+        {notification.type.toString()}
       </h2>
     </div>
     <button on:click|stopPropagation={discard}>
@@ -102,6 +72,6 @@
   </div>
 
   <p class="whitespace-pre-wrap pl-[28px] pr-[16px] text-sm" data-testid="message">
-    {notificationInfo.message}
+    {notification.message}
   </p>
 </div>
diff --git a/web/src/lib/components/shared-components/notification/notification-list.svelte b/web/src/lib/components/shared-components/notification/notification-list.svelte
@@ -11,9 +11,9 @@
 
 {#if $notificationList.length > 0}
   <section transition:fade={{ duration: 250 }} id="notification-list" class="fixed right-5 top-[80px] z-[99999999]">
-    {#each $notificationList as notificationInfo (notificationInfo.id)}
+    {#each $notificationList as notification (notification.id)}
       <div animate:flip={{ duration: 250, easing: quintOut }}>
-        <NotificationCard {notificationInfo} />
+        <NotificationCard {notification} />
       </div>
     {/each}
   </section>

diff --git a/web/src/lib/components/shared-components/notification/notification.ts b/web/src/lib/components/shared-components/notification/notification.ts
@@ -6,57 +6,43 @@ export enum NotificationType {
   Warning = 'Warning',
 }
 
-export class ImmichNotification {
-  id = Date.now() + Math.random();
-  type!: NotificationType;
-  message!: string;
-  action!: NotificationAction;
-  timeout = 3000;
-}
+export type Notification = {
+  id: number;
+  type: NotificationType;
+  message: string;
+  /** The action to take when the notification is clicked */
+  action: NotificationAction;
+  /** Timeout in miliseconds */
+  timeout: number;
+};
 
 type DiscardAction = { type: 'discard' };
 type NoopAction = { type: 'noop' };
 type LinkAction = { type: 'link'; target: string };
 export type NotificationAction = DiscardAction | NoopAction | LinkAction;
 
-export class ImmichNotificationDto {
-  /**
-   * Notification type
-   * @type {NotificationType} [Info, Error]
-   */
-  type: NotificationType = NotificationType.Info;
-
-  /**
-   * Notification message
-   */
-  message = '';
-
-  /**
-   * Timeout in miliseconds
-   */
-  timeout?: number;
-
-  /**
-   * The action to take when the notification is clicked
-   */
-  action?: NotificationAction;
-}
+export type NotificationOptions = Partial<Exclude<Notification, 'id'>> & { message: string };
 
 function createNotificationList() {
-  const notificationList = writable<ImmichNotification[]>([]);
-
-  const show = (notificationInfo: ImmichNotificationDto) => {
-    const newNotification = new ImmichNotification();
-    newNotification.message = notificationInfo.message;
-    newNotification.type = notificationInfo.type;
-    newNotification.timeout = notificationInfo.timeout || 3000;
-    newNotification.action = notificationInfo.action || { type: 'discard' };
-
-    notificationList.update((currentList) => [...currentList, newNotification]);
+  const notificationList = writable<Notification[]>([]);
+  let count = 1;
+
+  const show = (options: NotificationOptions) => {
+    notificationList.update((currentList) => {
+      currentList.push({
+        id: count++,
+        type: NotificationType.Info,
+        action: { type: 'discard' },
+        timeout: 3000,
+        ...options,
+      });
+
+      return currentList;
+    });
   };
 
   const removeNotificationById = (id: number) => {
-    notificationList.update((currentList) => currentList.filter((n) => n.id != id));
+    notificationList.update((currentList) => currentList.filter((n) => n.id !== id));
   };
 
   return {