From 537544169e380d3719f353b57caa1a010abc93dd Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 12 Jul 2024 00:10:11 -0400 Subject: [PATCH 1/3] feat: support cmd/ctrl click opening --- src/components/NotificationRow.tsx | 42 ++++++++++++++++-------------- src/utils/helpers.ts | 11 +++++--- src/utils/links.ts | 7 +++-- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 11f6749e5..6355a660c 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -16,6 +16,7 @@ import { getNotificationTypeIconColor, } from '../utils/icons'; import { openNotification } from '../utils/links'; +import { isMacOS } from '../utils/platform'; import { HoverGroup } from './HoverGroup'; import { InteractionButton } from './buttons/InteractionButton'; import { NotificationFooter } from './notification/NotificationFooter'; @@ -38,24 +39,27 @@ export const NotificationRow: FC = ({ const [animateExit, setAnimateExit] = useState(false); const [showAsRead, setShowAsRead] = useState(false); - const handleNotification = useCallback(() => { - setAnimateExit(!settings.delayNotificationState); - setShowAsRead(settings.delayNotificationState); - - openNotification(notification); - - if (settings.markAsDoneOnOpen) { - markNotificationDone(notification); - } else { - // no need to mark as read, github does it by default when opening it - removeNotificationFromState(settings, notification); - } - }, [ - notification, - markNotificationDone, - removeNotificationFromState, - settings, - ]); + const handleNotification = useCallback( + (event: MouseEvent) => { + // If the user is on a Mac, use the command key, otherwise use the control key. + const isCmdOrCtrlHeld = isMacOS() ? event.metaKey : event.ctrlKey; + + if (!isCmdOrCtrlHeld) { + setAnimateExit(!settings.delayNotificationState); + setShowAsRead(settings.delayNotificationState); + } + + openNotification(notification, !isCmdOrCtrlHeld); + + if (settings.markAsDoneOnOpen) { + markNotificationDone(notification); + } else if (!isCmdOrCtrlHeld) { + // no need to mark as read, github does it by default when opening it + removeNotificationFromState(settings, notification); + } + }, + [notification, markNotificationDone, removeNotificationFromState, settings], + ); const unsubscribeFromThread = (event: MouseEvent) => { // Don't trigger onClick of parent element. event.stopPropagation(); @@ -102,7 +106,7 @@ export const NotificationRow: FC = ({
handleNotification()} + onClick={(event) => handleNotification(event)} > diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index 31f047f0b..fad3c4b4e 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -100,6 +100,7 @@ async function getDiscussionUrl(notification: Notification): Promise { export async function generateGitHubWebUrl( notification: Notification, + withReferer = true, ): Promise { const url = new URL(notification.repository.html_url); @@ -136,10 +137,12 @@ export async function generateGitHubWebUrl( } } - url.searchParams.set( - 'notification_referrer_id', - generateNotificationReferrerId(notification), - ); + if (withReferer) { + url.searchParams.set( + 'notification_referrer_id', + generateNotificationReferrerId(notification), + ); + } return url.toString() as Link; } diff --git a/src/utils/links.ts b/src/utils/links.ts index 6edd44772..b0da94428 100644 --- a/src/utils/links.ts +++ b/src/utils/links.ts @@ -50,8 +50,11 @@ export function openRepository(repository: Repository) { openExternalLink(repository.html_url); } -export async function openNotification(notification: Notification) { - const url = await generateGitHubWebUrl(notification); +export async function openNotification( + notification: Notification, + withReferer = true, +) { + const url = await generateGitHubWebUrl(notification, withReferer); openExternalLink(url); } From 7b362aee619ee0965bb9c8c887042f7a8e9f9622 Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Fri, 12 Jul 2024 00:21:42 -0400 Subject: [PATCH 2/3] feat: support cmd/ctrl click opening --- src/components/NotificationRow.tsx | 2 +- src/utils/helpers.ts | 11 ++++------- src/utils/links.ts | 7 ++----- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index 6355a660c..c7ea5f804 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -49,7 +49,7 @@ export const NotificationRow: FC = ({ setShowAsRead(settings.delayNotificationState); } - openNotification(notification, !isCmdOrCtrlHeld); + openNotification(notification); if (settings.markAsDoneOnOpen) { markNotificationDone(notification); diff --git a/src/utils/helpers.ts b/src/utils/helpers.ts index fad3c4b4e..31f047f0b 100644 --- a/src/utils/helpers.ts +++ b/src/utils/helpers.ts @@ -100,7 +100,6 @@ async function getDiscussionUrl(notification: Notification): Promise { export async function generateGitHubWebUrl( notification: Notification, - withReferer = true, ): Promise { const url = new URL(notification.repository.html_url); @@ -137,12 +136,10 @@ export async function generateGitHubWebUrl( } } - if (withReferer) { - url.searchParams.set( - 'notification_referrer_id', - generateNotificationReferrerId(notification), - ); - } + url.searchParams.set( + 'notification_referrer_id', + generateNotificationReferrerId(notification), + ); return url.toString() as Link; } diff --git a/src/utils/links.ts b/src/utils/links.ts index b0da94428..6edd44772 100644 --- a/src/utils/links.ts +++ b/src/utils/links.ts @@ -50,11 +50,8 @@ export function openRepository(repository: Repository) { openExternalLink(repository.html_url); } -export async function openNotification( - notification: Notification, - withReferer = true, -) { - const url = await generateGitHubWebUrl(notification, withReferer); +export async function openNotification(notification: Notification) { + const url = await generateGitHubWebUrl(notification); openExternalLink(url); } From e70a8dcbbb162e00a7c9553b3593f9595f24f5ce Mon Sep 17 00:00:00 2001 From: Adam Setch Date: Sat, 13 Jul 2024 08:36:55 -0400 Subject: [PATCH 3/3] open in foreground/background --- src/__mocks__/utils.ts | 2 +- src/components/NotificationRow.test.tsx | 67 +++++++++++++++++++++++++ src/components/NotificationRow.tsx | 6 ++- src/routes/Settings.test.tsx | 4 +- src/utils/comms.test.ts | 4 +- src/utils/comms.ts | 6 ++- src/utils/links.test.ts | 21 ++++++-- src/utils/links.ts | 7 ++- 8 files changed, 104 insertions(+), 13 deletions(-) diff --git a/src/__mocks__/utils.ts b/src/__mocks__/utils.ts index 22149c55f..c4c0c7561 100644 --- a/src/__mocks__/utils.ts +++ b/src/__mocks__/utils.ts @@ -1,4 +1,4 @@ -export function mockPlatform(platform: NodeJS.Platform) { +export function setPlatform(platform: NodeJS.Platform) { Object.defineProperty(process, 'platform', { value: platform, }); diff --git a/src/components/NotificationRow.test.tsx b/src/components/NotificationRow.test.tsx index 15a6abb5b..61c6b5454 100644 --- a/src/components/NotificationRow.test.tsx +++ b/src/components/NotificationRow.test.tsx @@ -4,6 +4,7 @@ import { mockGitHubCloudAccount, mockSettings, } from '../__mocks__/state-mocks'; +import { setPlatform } from '../__mocks__/utils'; import { AppContext } from '../context/App'; import { GroupBy, type Link } from '../types'; import type { UserType } from '../typesGitHub'; @@ -82,6 +83,18 @@ describe('components/NotificationRow.tsx', () => { }); describe('notification interactions', () => { + let originalPlatform: NodeJS.Platform; + + beforeEach(() => { + // Save the original platform value + originalPlatform = process.platform; + }); + + afterEach(() => { + // Restore the original platform value + setPlatform(originalPlatform); + }); + it('should open a notification in the browser - click', () => { const removeNotificationFromState = jest.fn(); @@ -107,6 +120,60 @@ describe('components/NotificationRow.tsx', () => { expect(removeNotificationFromState).toHaveBeenCalledTimes(1); }); + it('should open a notification in the browser - macOS meta key click', () => { + setPlatform('darwin'); + + const removeNotificationFromState = jest.fn(); + + const props = { + notification: mockSingleNotification, + account: mockGitHubCloudAccount, + }; + + render( + + + , + ); + + fireEvent.click(screen.getByRole('main'), { metaKey: true }); + expect(links.openNotification).toHaveBeenCalledTimes(1); + expect(removeNotificationFromState).toHaveBeenCalledTimes(0); + }); + + it('should open a notification in the browser - ctrl key click', () => { + setPlatform('win32'); + + const removeNotificationFromState = jest.fn(); + + const props = { + notification: mockSingleNotification, + account: mockGitHubCloudAccount, + }; + + render( + + + , + ); + + fireEvent.click(screen.getByRole('main'), { ctrlKey: true }); + expect(links.openNotification).toHaveBeenCalledTimes(1); + expect(removeNotificationFromState).toHaveBeenCalledTimes(0); + }); + it('should open a notification in the browser - delay notification setting enabled', () => { const removeNotificationFromState = jest.fn(); diff --git a/src/components/NotificationRow.tsx b/src/components/NotificationRow.tsx index c7ea5f804..12735094b 100644 --- a/src/components/NotificationRow.tsx +++ b/src/components/NotificationRow.tsx @@ -49,11 +49,13 @@ export const NotificationRow: FC = ({ setShowAsRead(settings.delayNotificationState); } - openNotification(notification); + openNotification(notification, !isCmdOrCtrlHeld); if (settings.markAsDoneOnOpen) { markNotificationDone(notification); - } else if (!isCmdOrCtrlHeld) { + } else if (isCmdOrCtrlHeld) { + setShowAsRead(settings.delayNotificationState); + } else { // no need to mark as read, github does it by default when opening it removeNotificationFromState(settings, notification); } diff --git a/src/routes/Settings.test.tsx b/src/routes/Settings.test.tsx index 23aaf2022..13b754b31 100644 --- a/src/routes/Settings.test.tsx +++ b/src/routes/Settings.test.tsx @@ -1,7 +1,7 @@ import { act, fireEvent, render, screen } from '@testing-library/react'; import { MemoryRouter } from 'react-router-dom'; import { mockAuth, mockSettings } from '../__mocks__/state-mocks'; -import { mockPlatform } from '../__mocks__/utils'; +import { setPlatform } from '../__mocks__/utils'; import { AppContext } from '../context/App'; import { SettingsRoute } from './Settings'; @@ -21,7 +21,7 @@ describe('routes/Settings.tsx', () => { beforeAll(() => { // Save the original platform value originalPlatform = process.platform; - mockPlatform('darwin'); + setPlatform('darwin'); }); afterEach(() => { diff --git a/src/utils/comms.test.ts b/src/utils/comms.test.ts index 0bde2bf60..a2dc03ce2 100644 --- a/src/utils/comms.test.ts +++ b/src/utils/comms.test.ts @@ -61,7 +61,9 @@ describe('utils/comms.ts', () => { it('should open an external link', () => { openExternalLink('http://www.gitify.io/' as Link); expect(shell.openExternal).toHaveBeenCalledTimes(1); - expect(shell.openExternal).toHaveBeenCalledWith('http://www.gitify.io/'); + expect(shell.openExternal).toHaveBeenCalledWith('http://www.gitify.io/', { + activate: true, + }); }); it('should ignore opening external local links file:///', () => { diff --git a/src/utils/comms.ts b/src/utils/comms.ts index e19bef005..7b4de09b0 100644 --- a/src/utils/comms.ts +++ b/src/utils/comms.ts @@ -2,9 +2,11 @@ import { ipcRenderer, shell } from 'electron'; import type { Link } from '../types'; import Constants from './constants'; -export function openExternalLink(url: Link): void { +export function openExternalLink(url: Link, openInForeground = true): void { if (!url.toLowerCase().startsWith('file:///')) { - shell.openExternal(url); + shell.openExternal(url, { + activate: openInForeground, + }); } } diff --git a/src/utils/links.test.ts b/src/utils/links.test.ts index 316fff367..5e2d2be9c 100644 --- a/src/utils/links.test.ts +++ b/src/utils/links.test.ts @@ -107,13 +107,28 @@ describe('utils/links.ts', () => { expect(comms.openExternalLink).toHaveBeenCalledWith(mockHtmlUrl); }); - it('openNotification', async () => { + it('openNotification - foreground', async () => { const mockNotificationUrl = mockSingleNotification.repository.html_url; jest .spyOn(helpers, 'generateGitHubWebUrl') .mockResolvedValue(mockNotificationUrl); - await openNotification(mockSingleNotification); - expect(comms.openExternalLink).toHaveBeenCalledWith(mockNotificationUrl); + await openNotification(mockSingleNotification, true); + expect(comms.openExternalLink).toHaveBeenCalledWith( + mockNotificationUrl, + true, + ); + }); + + it('openNotification - background', async () => { + const mockNotificationUrl = mockSingleNotification.repository.html_url; + jest + .spyOn(helpers, 'generateGitHubWebUrl') + .mockResolvedValue(mockNotificationUrl); + await openNotification(mockSingleNotification, false); + expect(comms.openExternalLink).toHaveBeenCalledWith( + mockNotificationUrl, + false, + ); }); it('openParticipatingDocs', () => { diff --git a/src/utils/links.ts b/src/utils/links.ts index 6edd44772..b784be0d6 100644 --- a/src/utils/links.ts +++ b/src/utils/links.ts @@ -50,9 +50,12 @@ export function openRepository(repository: Repository) { openExternalLink(repository.html_url); } -export async function openNotification(notification: Notification) { +export async function openNotification( + notification: Notification, + openInForeground?: boolean, +) { const url = await generateGitHubWebUrl(notification); - openExternalLink(url); + openExternalLink(url, openInForeground); } export function openGitHubParticipatingDocs() {