Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: show "read" notifications #1052

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/__mocks__/mock-state.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,4 +18,5 @@ export const mockSettings: SettingsState = {
detailedNotifications: false,
markAsDoneOnOpen: false,
showAccountHostname: false,
showAllNotifications: false,
};
68 changes: 50 additions & 18 deletions src/components/NotificationRow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
import { AppContext } from '../context/App';
import type { Notification } from '../typesGithub';
import { openExternalLink } from '../utils/comms';
import Constants from '../utils/constants';
import { formatForDisplay, openInBrowser } from '../utils/helpers';
import {
getNotificationTypeIcon,
Expand All @@ -32,9 +33,9 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {
const {
settings,
accounts,
removeNotificationFromState,
markNotificationRead,
markNotificationDone,
removeNotificationFromState,
unsubscribeNotification,
notifications,
} = useContext(AppContext);
Expand All @@ -44,17 +45,27 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {

if (settings.markAsDoneOnOpen) {
markNotificationDone(notification.id, hostname);
} else {
// no need to mark as read, github does it by default when opening it
removeNotificationFromState(notification.id, hostname);
}
handleNotificationState();
}, [notifications, notification, accounts, settings]); // notifications required here to prevent weird state issues

const unsubscribe = (event: MouseEvent<HTMLElement>) => {
// Don't trigger onClick of parent element.
event.stopPropagation();

unsubscribeNotification(notification.id, hostname);
markNotificationRead(notification.id, hostname);
handleNotificationState();
};

const markAsRead = () => {
markNotificationRead(notification.id, hostname);
handleNotificationState();
};

const markAsDone = () => {
markNotificationDone(notification.id, hostname);
handleNotificationState();
};

const openUserProfile = (
Expand All @@ -66,6 +77,20 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {
openExternalLink(notification.subject.user.html_url);
};

const handleNotificationState = useCallback(() => {
if (!settings.showAllNotifications) {
removeNotificationFromState(notification.id, hostname);
}

if (notification.unread) {
const notificationRow = document.getElementById(notification.id);
notificationRow.className += Constants.READ_CLASS_NAME;
}

// TODO FIXME - this is not updating the notification count
notification.unread = false;
}, [notification, notifications]);

const reason = formatReason(notification.reason);
const NotificationIcon = getNotificationTypeIcon(notification.subject);
const iconColor = getNotificationTypeIconColor(notification.subject);
Expand All @@ -82,18 +107,21 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {
]);

return (
<div className="flex space-x-3 py-2 px-3 bg-white dark:bg-gray-dark dark:text-white hover:bg-gray-100 dark:hover:bg-gray-darker border-b border-gray-100 dark:border-gray-darker group">
<div
id={notification.id}
className={`flex space-x-3 py-2 px-3 bg-white border-b border-gray-100 dark:border-gray-darker group dark:bg-gray-dark dark:text-white hover:bg-gray-100 dark:hover:bg-gray-darker
${!notification.unread ? Constants.READ_CLASS_NAME : ''}`}
>
<div
className={`flex justify-center items-center w-5 ${iconColor}`}
className={`flex flex-col justify-center items-center w-5 ${iconColor}`}
title={notificationTitle}
>
<NotificationIcon size={18} aria-label={notification.subject.type} />
</div>

<div
className="flex-1 overflow-hidden"
onClick={() => openNotification()}
onKeyDown={() => openNotification()}
onClick={openNotification}
onKeyDown={openNotification}
>
<div
className="mb-1 text-sm whitespace-nowrap overflow-ellipsis overflow-hidden cursor-pointer"
Expand Down Expand Up @@ -141,7 +169,7 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {
type="button"
className="focus:outline-none h-full hover:text-green-500"
title="Mark as Done"
onClick={() => markNotificationDone(notification.id, hostname)}
onClick={markAsDone}
>
<CheckIcon size={16} aria-label="Mark as Done" />
</button>
Expand All @@ -155,14 +183,18 @@ export const NotificationRow: FC<IProps> = ({ notification, hostname }) => {
<BellSlashIcon size={14} aria-label="Unsubscribe" />
</button>

<button
type="button"
className="focus:outline-none h-full hover:text-green-500"
title="Mark as Read"
onClick={() => markNotificationRead(notification.id, hostname)}
>
<ReadIcon size={14} aria-label="Mark as Read" />
</button>
{notification.unread ? (
<button
type="button"
className="focus:outline-none h-full hover:text-green-500"
title="Mark as Read"
onClick={markAsRead}
>
<ReadIcon size={14} aria-label="Mark as Read" />
</button>
) : (
<div className="w-[14px]" />
)}
</div>
</div>
);
Expand Down
8 changes: 4 additions & 4 deletions src/components/Repository.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ jest.mock('./NotificationRow', () => ({
}));

describe('components/Repository.tsx', () => {
const markRepoNotifications = jest.fn();
const markRepoNotificationsRead = jest.fn();
const markRepoNotificationsDone = jest.fn();

const props = {
Expand All @@ -20,7 +20,7 @@ describe('components/Repository.tsx', () => {
};

beforeEach(() => {
markRepoNotifications.mockReset();
markRepoNotificationsRead.mockReset();

jest.spyOn(shell, 'openExternal');
});
Expand Down Expand Up @@ -51,14 +51,14 @@ describe('components/Repository.tsx', () => {

it('should mark a repo as read', () => {
render(
<AppContext.Provider value={{ markRepoNotifications }}>
<AppContext.Provider value={{ markRepoNotificationsRead }}>
<RepositoryNotifications {...props} />
</AppContext.Provider>,
);

fireEvent.click(screen.getByTitle('Mark Repository as Read'));

expect(markRepoNotifications).toHaveBeenCalledWith(
expect(markRepoNotificationsRead).toHaveBeenCalledWith(
'manosim/notifications-test',
'github.com',
);
Expand Down
53 changes: 42 additions & 11 deletions src/components/Repository.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { CSSTransition, TransitionGroup } from 'react-transition-group';
import { AppContext } from '../context/App';
import type { Notification } from '../typesGithub';
import { openExternalLink } from '../utils/comms';
import Constants from '../utils/constants';
import { NotificationRow } from './NotificationRow';

interface IProps {
Expand All @@ -19,8 +20,13 @@ export const RepositoryNotifications: FC<IProps> = ({
repoNotifications,
hostname,
}) => {
const { markRepoNotifications, markRepoNotificationsDone } =
useContext(AppContext);
const {
markRepoNotificationsRead,
markRepoNotificationsDone,
settings,
notifications,
removeNotificationsFromState,
} = useContext(AppContext);

const openBrowser = useCallback(() => {
const url = repoNotifications[0].repository.html_url;
Expand All @@ -29,14 +35,35 @@ export const RepositoryNotifications: FC<IProps> = ({

const markRepoAsRead = useCallback(() => {
const repoSlug = repoNotifications[0].repository.full_name;
markRepoNotifications(repoSlug, hostname);
markRepoNotificationsRead(repoSlug, hostname);
handleNotificationState(repoSlug);
}, [repoNotifications, hostname]);

const markRepoAsDone = useCallback(() => {
const repoSlug = repoNotifications[0].repository.full_name;
markRepoNotificationsDone(repoSlug, hostname);
handleNotificationState(repoSlug);
}, [repoNotifications, hostname]);

const handleNotificationState = useCallback(
(repoSlug: string) => {
if (!settings.showAllNotifications) {
removeNotificationsFromState(repoSlug, notifications, hostname);
}

for (const notification of repoNotifications) {
if (notification.unread) {
const notificationRow = document.getElementById(notification.id);
notificationRow.className += Constants.READ_CLASS_NAME;
}

// TODO FIXME - this is not updating the notification count
notification.unread = false;
}
},
[repoNotifications, hostname, notifications],
);

const avatarUrl = repoNotifications[0].repository.owner.avatar_url;
const repoSlug = repoNotifications[0].repository.full_name;

Expand Down Expand Up @@ -74,14 +101,18 @@ export const RepositoryNotifications: FC<IProps> = ({

<div className="w-[14px]" />

<button
type="button"
className="focus:outline-none h-full hover:text-green-500"
title="Mark Repository as Read"
onClick={markRepoAsRead}
>
<ReadIcon size={14} aria-label="Mark Repository as Read" />
</button>
{repoNotifications.some((notification) => notification.unread) ? (
<button
type="button"
className="focus:outline-none h-full hover:text-green-500"
title="Mark Repository as Read"
onClick={markRepoAsRead}
>
<ReadIcon size={14} aria-label="Mark Repository as Read" />
</button>
) : (
<div className="w-[14px]" />
)}
</div>
</div>

Expand Down
4 changes: 2 additions & 2 deletions src/components/Sidebar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { Logo } from '../components/Logo';
import { AppContext } from '../context/App';
import { openExternalLink } from '../utils/comms';
import { Constants } from '../utils/constants';
import { getNotificationCount } from '../utils/notifications';
import { getUnreadNotificationCount } from '../utils/notifications';

export const Sidebar: FC = () => {
const navigate = useNavigate();
Expand All @@ -35,7 +35,7 @@ export const Sidebar: FC = () => {
}, []);

const notificationsCount = useMemo(() => {
return getNotificationCount(notifications);
return getUnreadNotificationCount(notifications);
}, [notifications]);

const sidebarButtonClasses =
Expand Down
12 changes: 6 additions & 6 deletions src/context/App.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,11 @@ describe('context/App.tsx', () => {

describe('api methods', () => {
const apiRequestAuthMock = jest.spyOn(apiRequests, 'apiRequestAuth');
const getNotificationCountMock = jest.spyOn(
const getUnreadNotificationCountMock = jest.spyOn(
notifications,
'getNotificationCount',
'getUnreadNotificationCount',
);
getNotificationCountMock.mockReturnValue(1);
getUnreadNotificationCountMock.mockReturnValue(1);

const fetchNotificationsMock = jest.fn();
const markNotificationReadMock = jest.fn();
Expand Down Expand Up @@ -193,15 +193,15 @@ describe('context/App.tsx', () => {
);
});

it('should call markRepoNotifications', async () => {
it('should call markRepoNotificationsRead', async () => {
const TestComponent = () => {
const { markRepoNotifications } = useContext(AppContext);
const { markRepoNotificationsRead } = useContext(AppContext);

return (
<button
type="button"
onClick={() =>
markRepoNotifications('manosim/gitify', 'github.com')
markRepoNotificationsRead('manosim/gitify', 'github.com')
}
>
Test Case
Expand Down
Loading
Loading