Repository: gitify-app/gitify
Pull request #746 (2024-02-06 11:17:50)

feat: add "Mark as done on open" setting
## Issues related

Implements #714 

## Context

This PR introduces a new setting which I called "Mark as done on open" (defaults to `false`).

It takes advantage of the new API to mark a notification as done, and replaces the old "Mark as read on click" setting (which was removed in #713 because it became obsolete).

The PR also includes test, I pretty much copy-pasted everything from #713 to implement both the feature and the test, so there's really nothing new 😉  

## Discussion

I decided to name the setting "Mark as done on **open**" instead of "Mark as done on **click**" because "click" felt too vague to me (click on what?)

This can be discussed.



Patch:
diff --git a/src/__mocks__/mock-state.ts b/src/__mocks__/mock-state.ts
index accdabd5..310581a6 100644
--- a/src/__mocks__/mock-state.ts
+++ b/src/__mocks__/mock-state.ts
@@ -19,4 +19,5 @@ export const mockSettings: SettingsState = {
   openAtStartup: false,
   appearance: Appearance.SYSTEM,
   colors: false,
+  markAsDoneOnOpen: false,
 };
diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx
index d8b58c72..b5ab1026 100644
--- a/src/components/NotificationRow.tsx
+++ b/src/components/NotificationRow.tsx
@@ -1,6 +1,6 @@
 import React, { useCallback, useContext } from 'react';
 import { formatDistanceToNow, parseISO } from 'date-fns';
-import { CheckIcon, MuteIcon, ReadIcon } from '@primer/octicons-react';
+import { CheckIcon, BellSlashIcon, ReadIcon } from '@primer/octicons-react';
 
 import {
   formatReason,
@@ -30,6 +30,10 @@ export const NotificationRow: React.FC<IProps> = ({
 
   const pressTitle = useCallback(() => {
     openBrowser();
+
+    if (settings.markAsDoneOnOpen) {
+      markNotificationDone(notification.id, hostname);
+    }
   }, [settings]);
 
   const openBrowser = useCallback(
@@ -72,21 +76,22 @@ export const NotificationRow: React.FC<IProps> = ({
         <div className="text-xs text-capitalize">
           <span title={reason.description}>{reason.type}</span> - Updated{' '}
           {updatedAt}
-          <button
-            className="border-0 bg-none float-right"
-            title="Unsubscribe"
-            onClick={unsubscribe}
-          >
-            <MuteIcon
-              className="hover:text-red-500"
-              size={13}
-              aria-label="Unsubscribe"
-            />
-          </button>
         </div>
       </div>
 
       <div className="flex justify-center items-center gap-2">
+        <button
+          className="focus:outline-none"
+          title="Mark as Done"
+          onClick={() => markNotificationDone(notification.id, hostname)}
+        >
+          <CheckIcon
+            className="hover:text-green-500"
+            size={16}
+            aria-label="Mark as Done"
+          />
+        </button>
+
         <button
           className="focus:outline-none"
           title="Mark as Read"
@@ -100,14 +105,14 @@ export const NotificationRow: React.FC<IProps> = ({
         </button>
 
         <button
-          className="focus:outline-none"
-          title="Mark as Done"
-          onClick={() => markNotificationDone(notification.id, hostname)}
+          className="border-0 bg-none float-right"
+          title="Unsubscribe"
+          onClick={unsubscribe}
         >
-          <CheckIcon
-            className="hover:text-green-500"
-            size={16}
-            aria-label="Mark as Done"
+          <BellSlashIcon
+            className="hover:text-red-500"
+            size={14}
+            aria-label="Unsubscribe"
           />
         </button>
       </div>
diff --git a/src/context/App.tsx b/src/context/App.tsx
index dbc788cd..d009a160 100644
--- a/src/context/App.tsx
+++ b/src/context/App.tsx
@@ -37,6 +37,7 @@ export const defaultSettings: SettingsState = {
   openAtStartup: false,
   appearance: Appearance.SYSTEM,
   colors: false,
+  markAsDoneOnOpen: false,
 };
 
 interface AppContextState {
diff --git a/src/routes/Settings.tsx b/src/routes/Settings.tsx
index 433c0e4e..78478e2f 100644
--- a/src/routes/Settings.tsx
+++ b/src/routes/Settings.tsx
@@ -139,6 +139,14 @@ export const SettingsRoute: React.FC = () => {
             updateSetting('showNotifications', evt.target.checked)
           }
         />
+        <FieldCheckbox
+          name="markAsDoneOnOpen"
+          label="Mark as done on open"
+          checked={settings.markAsDoneOnOpen}
+          onChange={(evt) =>
+            updateSetting('markAsDoneOnOpen', evt.target.checked)
+          }
+        />
         {!isLinux && (
           <FieldCheckbox
             name="openAtStartUp"
diff --git a/src/types.ts b/src/types.ts
index 8aac5134..5925a24f 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -13,6 +13,7 @@ export interface SettingsState {
   openAtStartup: boolean;
   appearance: Appearance;
   colors: boolean;
+  markAsDoneOnOpen: boolean;
 }
 
 export enum Appearance {


Test patch:
diff --git a/src/components/NotificationRow.test.tsx b/src/components/NotificationRow.test.tsx
index 0359f0ee..dcd601b1 100644
--- a/src/components/NotificationRow.test.tsx
+++ b/src/components/NotificationRow.test.tsx
@@ -41,7 +41,7 @@ describe('components/Notification.js', () => {
     const { getByRole } = render(
       <AppContext.Provider
         value={{
-          settings: mockSettings,
+          settings: { ...mockSettings, markAsDoneOnOpen: false },
           markNotification,
           accounts: mockAccounts,
         }}
@@ -54,6 +54,31 @@ describe('components/Notification.js', () => {
     expect(shell.openExternal).toHaveBeenCalledTimes(1);
   });
 
+  it('should open a notification in browser & mark it as done', () => {
+    const markNotificationDone = jest.fn();
+
+    const props = {
+      notification: mockedSingleNotification,
+      hostname: 'github.com',
+    };
+
+    const { getByRole } = render(
+      <AppContext.Provider
+        value={{
+          settings: { ...mockSettings, markAsDoneOnOpen: true },
+          markNotificationDone,
+          accounts: mockAccounts,
+        }}
+      >
+        <NotificationRow {...props} />
+      </AppContext.Provider>,
+    );
+
+    fireEvent.click(getByRole('main'));
+    expect(shell.openExternal).toHaveBeenCalledTimes(1);
+    expect(markNotificationDone).toHaveBeenCalledTimes(1);
+  });
+
   it('should mark a notification as read', () => {
     const markNotification = jest.fn();
 
@@ -65,7 +90,7 @@ describe('components/Notification.js', () => {
     const { getByTitle } = render(
       <AppContext.Provider
         value={{
-          settings: mockSettings,
+          settings: { ...mockSettings, markAsDoneOnOpen: false },
           accounts: mockAccounts,
         }}
       >
diff --git a/src/components/__snapshots__/NotificationRow.test.tsx.snap b/src/components/__snapshots__/NotificationRow.test.tsx.snap
index 9d9ed504..635848ad 100644
--- a/src/components/__snapshots__/NotificationRow.test.tsx.snap
+++ b/src/components/__snapshots__/NotificationRow.test.tsx.snap
@@ -55,40 +55,40 @@ exports[`components/Notification.js should render itself & its children 1`] = `
        - Updated
        
       in over 3 years
-      <button
-        className="border-0 bg-none float-right"
-        onClick={[Function]}
-        title="Unsubscribe"
-      >
-        <svg
-          aria-hidden="false"
-          aria-label="Unsubscribe"
-          className="hover:text-red-500"
-          fill="currentColor"
-          focusable="false"
-          height={13}
-          role="img"
-          style={
-            {
-              "display": "inline-block",
-              "overflow": "visible",
-              "userSelect": "none",
-              "verticalAlign": "text-bottom",
-            }
-          }
-          viewBox="0 0 16 16"
-          width={13}
-        >
-          <path
-            d="M8 2.75v10.5a.751.751 0 0 1-1.238.57L3.473 11H1.75A1.75 1.75 0 0 1 0 9.25v-2.5C0 5.784.784 5 1.75 5h1.722l3.29-2.82A.75.75 0 0 1 8 2.75Zm3.28 2.47L13 6.94l1.72-1.72a.751.751 0 0 1 1.042.018.751.751 0 0 1 .018 1.042L14.06 8l1.72 1.72a.749.749 0 0 1-.326 1.275.749.749 0 0 1-.734-.215L13 9.06l-1.72 1.72a.749.749 0 0 1-1.275-.326.749.749 0 0 1 .215-.734L11.94 8l-1.72-1.72a.749.749 0 0 1 .326-1.275.749.749 0 0 1 .734.215Zm-7.042 1.1a.752.752 0 0 1-.488.18h-2a.25.25 0 0 0-.25.25v2.5c0 .138.112.25.25.25h2c.179 0 .352.064.488.18L6.5 11.62V4.38Z"
-          />
-        </svg>
-      </button>
     </div>
   </div>
   <div
     className="flex justify-center items-center gap-2"
   >
+    <button
+      className="focus:outline-none"
+      onClick={[Function]}
+      title="Mark as Done"
+    >
+      <svg
+        aria-hidden="false"
+        aria-label="Mark as Done"
+        className="hover:text-green-500"
+        fill="currentColor"
+        focusable="false"
+        height={16}
+        role="img"
+        style={
+          {
+            "display": "inline-block",
+            "overflow": "visible",
+            "userSelect": "none",
+            "verticalAlign": "text-bottom",
+          }
+        }
+        viewBox="0 0 16 16"
+        width={16}
+      >
+        <path
+          d="M13.78 4.22a.75.75 0 0 1 0 1.06l-7.25 7.25a.75.75 0 0 1-1.06 0L2.22 9.28a.751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018L6 10.94l6.72-6.72a.75.75 0 0 1 1.06 0Z"
+        />
+      </svg>
+    </button>
     <button
       className="focus:outline-none"
       onClick={[Function]}
@@ -119,17 +119,17 @@ exports[`components/Notification.js should render itself & its children 1`] = `
       </svg>
     </button>
     <button
-      className="focus:outline-none"
+      className="border-0 bg-none float-right"
       onClick={[Function]}
-      title="Mark as Done"
+      title="Unsubscribe"
     >
       <svg
         aria-hidden="false"
-        aria-label="Mark as Done"
-        className="hover:text-green-500"
+        aria-label="Unsubscribe"
+        className="hover:text-red-500"
         fill="currentColor"
         focusable="false"
-        height={16}
+        height={14}
         role="img"
         style={
           {
@@ -140,10 +140,10 @@ exports[`components/Notification.js should render itself & its children 1`] = `
           }
         }
         viewBox="0 0 16 16"
-        width={16}
+        width={14}
       >
         <path
-          d="M13.78 4.22a.75.75 0 0 1 0 1.06l-7.25 7.25a.75.75 0 0 1-1.06 0L2.22 9.28a.751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018L6 10.94l6.72-6.72a.75.75 0 0 1 1.06 0Z"
+          d="m4.182 4.31.016.011 10.104 7.316.013.01 1.375.996a.75.75 0 1 1-.88 1.214L13.626 13H2.518a1.516 1.516 0 0 1-1.263-2.36l1.703-2.554A.255.255 0 0 0 3 7.947V5.305L.31 3.357a.75.75 0 1 1 .88-1.214Zm7.373 7.19L4.5 6.391v1.556c0 .346-.102.683-.294.97l-1.703 2.556a.017.017 0 0 0-.003.01c0 .005.002.009.005.012l.006.004.007.001ZM8 1.5c-.997 0-1.895.416-2.534 1.086A.75.75 0 1 1 4.38 1.55 5 5 0 0 1 13 5v2.373a.75.75 0 0 1-1.5 0V5A3.5 3.5 0 0 0 8 1.5ZM8 16a2 2 0 0 1-1.985-1.75c-.017-.137.097-.25.235-.25h3.5c.138 0 .252.113.235.25A2 2 0 0 1 8 16Z"
         />
       </svg>
     </button>
diff --git a/src/context/App.test.tsx b/src/context/App.test.tsx
index aea2d742..1c788a9e 100644
--- a/src/context/App.test.tsx
+++ b/src/context/App.test.tsx
@@ -291,6 +291,7 @@ describe('context/App.tsx', () => {
         playSound: true,
         showNotifications: true,
         colors: false,
+        markAsDoneOnOpen: false,
       },
     );
   });
@@ -328,6 +329,7 @@ describe('context/App.tsx', () => {
         playSound: true,
         showNotifications: true,
         colors: false,
+        markAsDoneOnOpen: false,
       },
     );
   });
diff --git a/src/routes/Settings.test.tsx b/src/routes/Settings.test.tsx
index d82baf90..933dd248 100644
--- a/src/routes/Settings.test.tsx
+++ b/src/routes/Settings.test.tsx
@@ -182,6 +182,34 @@ describe('routes/Settings.tsx', () => {
     expect(updateSetting).toHaveBeenCalledWith('showNotifications', false);
   });
 
+  it('should toggle the markAsDoneOnOpen checkbox', async () => {
+    let getByLabelText;
+
+    await act(async () => {
+      const { getByLabelText: getByLabelTextLocal } = render(
+        <AppContext.Provider
+          value={{
+            settings: mockSettings,
+            accounts: mockAccounts,
+            updateSetting,
+          }}
+        >
+          <MemoryRouter>
+            <SettingsRoute />
+          </MemoryRouter>
+        </AppContext.Provider>,
+      );
+      getByLabelText = getByLabelTextLocal;
+    });
+
+    fireEvent.click(getByLabelText('Mark as done on open'), {
+      target: { checked: true },
+    });
+
+    expect(updateSetting).toHaveBeenCalledTimes(1);
+    expect(updateSetting).toHaveBeenCalledWith('markAsDoneOnOpen', false);
+  });
+
   it('should toggle the openAtStartup checkbox', async () => {
     let getByLabelText;
 
diff --git a/src/routes/__snapshots__/Settings.test.tsx.snap b/src/routes/__snapshots__/Settings.test.tsx.snap
index a11bcec6..985fcb40 100644
--- a/src/routes/__snapshots__/Settings.test.tsx.snap
+++ b/src/routes/__snapshots__/Settings.test.tsx.snap
@@ -230,6 +230,29 @@ exports[`routes/Settings.tsx should render itself & its children 1`] = `
         </label>
       </div>
     </div>
+    <div
+      class="flex items-start mt-1 mb-3"
+    >
+      <div
+        class="flex items-center h-5"
+      >
+        <input
+          class="focus:ring-indigo-500 h-4 w-4 text-indigo-600 border-gray-300 rounded"
+          id="markAsDoneOnOpen"
+          type="checkbox"
+        />
+      </div>
+      <div
+        class="ml-3 text-sm"
+      >
+        <label
+          class="font-medium text-gray-700 dark:text-gray-200"
+          for="markAsDoneOnOpen"
+        >
+          Mark as done on open
+        </label>
+      </div>
+    </div>
     <div
       class="flex items-start mt-1 mb-3"
     >