Add NotificationContext and useNotification hook #524

Griffin-Sullivan · 2024-10-28T19:29:45Z

Description

This PR adds a new global context for Notifications. I also added a useNotification hook to allow for easier dispatching by calling the specific method you want to use notification.success or notification.error. I also looped in the removal of notifications into this hook. You can call it like:

notifications = useNotification() // returns state for notifications
...
notifications.remove(notification.id)

I still not have tested restoring an archived version yet since I can't do it in mocked mode

There are a couple known issues at the moment:

The id count gets reset to 0 whenever you change pages meaning if you switch pages quickly and get another notification they will have the same id
The notification exit button looks out of place (I think it should be center aligned not in the top right corner)

How Has This Been Tested?

Merge criteria:

All the commits have been signed-off (To pass the DCO check)

The commits have meaningful messages; the author will squash them after approval or in case of manual merges will ask to merge with squash.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work.
Code changes follow the kubeflow contribution guidelines.

If you have UI changes

The developer has added tests or explained why testing cannot be added.
Included any necessary screenshots or gifs if it was a UI change.
Verify that UI/UX changes conform the UX guidelines for Kubeflow.

Griffin-Sullivan · 2024-10-28T19:31:04Z

clients/ui/frontend/src/app/hooks/useNotification.ts

+interface NotificationFunc extends NotificationTypeFunc {
+  remove: NotificationRemoveProps
+}


This is a little hacky. Maybe we should just have a separate hook for when we want to return all notifications and do something with them like removing one.

clients/ui/frontend/src/components/ToastNotification.tsx

Griffin-Sullivan · 2024-10-28T19:33:41Z

clients/ui/frontend/src/components/ToastNotifications.tsx

+import { NotificationContext } from '~/app/context/NotificationContext';
+
+const ToastNotifications: React.FC = () => {
+  const [notifications] = useContext(NotificationContext);


The current hook only gives access to notification actions. Should we expand it to also return the notifications list? This is why I'm using useContext here.

lucferbux

Haven't tested it yet, but did a quick review, we can take a deeper look later today if you want.

clients/ui/frontend/src/utilities/utils.ts

clients/ui/frontend/src/app/context/NotificationContext.tsx

clients/ui/frontend/src/components/ToastNotification.tsx

Griffin-Sullivan · 2024-10-29T17:40:02Z

Ok @lucferbux addressed your comments thanks!

Griffin-Sullivan · 2024-11-04T20:42:31Z

Ok with the newest push I have added a bunch of testing which includes some for checking the notification pops up correctly.

Griffin-Sullivan · 2024-11-04T20:49:54Z

clients/ui/frontend/src/__tests__/cypress/cypress/pages/components/Notification.ts

@@ -0,0 +1,7 @@
+export class Notification {


Maybe we should rename ToastNotification?

Yes, we are not using Notification, we might even refactor this in the future if this is not part of the project but reusable code, we'll see

At the moment, just rename it and we can call it a day

Griffin-Sullivan · 2024-11-04T20:51:41Z

clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelVersionArchive.cy.ts

+          modelVersionId: 2,
+        },
+      },
+      mockBFFResponse(mockModelVersion({})),


Notice how we have to put a bunch of mockBFFResponse everywhere. We should figure out a way for all of our mockObject calls to inherently know about the BFF response so we don't have to be so explicit.

This is a great point, my first guess is to just modifify the mock objects and add a wrapper there, can you open a ticket in enhancemets to cover this?

lucferbux

Everything looks great, just a couple of nits, really great work here!

lucferbux · 2024-11-06T11:31:18Z

clients/ui/frontend/src/__tests__/cypress/cypress/pages/components/Notification.ts

@@ -0,0 +1,7 @@
+export class Notification {


Yes, we are not using Notification, we might even refactor this in the future if this is not part of the project but reusable code, we'll see

lucferbux · 2024-11-06T11:31:40Z

clients/ui/frontend/src/__tests__/cypress/cypress/pages/components/Notification.ts

@@ -0,0 +1,7 @@
+export class Notification {


At the moment, just rename it and we can call it a day

lucferbux · 2024-11-06T11:33:46Z

clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelVersionArchive.cy.ts

+          modelVersionId: 2,
+        },
+      },
+      mockBFFResponse(mockModelVersion({})),


This is a great point, my first guess is to just modifify the mock objects and add a wrapper there, can you open a ticket in enhancemets to cover this?

lucferbux · 2024-11-06T11:42:40Z

clients/ui/frontend/src/app/context/NotificationContext.tsx

+export type Notification = {
+  id?: number;
+  status: AlertVariant;
+  title: string;
+  message?: React.ReactNode;
+  hidden?: boolean;
+  read?: boolean;
+  timestamp: Date;
+};
+
+export enum NotificationActionTypes {
+  ADD_NOTIFICATION = 'add_notification',
+  DELETE_NOTIFICATION = 'delete_notification',
+}
+
+type NotificationAction =
+  | {
+      type: NotificationActionTypes.ADD_NOTIFICATION;
+      payload: Notification;
+    }
+  | {
+      type: NotificationActionTypes.DELETE_NOTIFICATION;
+      payload: { id: Notification['id'] };
+    };


Can you move this to the types.ts file under app?

lucferbux · 2024-11-06T11:43:43Z

clients/ui/frontend/src/app/hooks/useNotification.ts

+
+enum NotificationTypes {
+  SUCCESS = 'success',
+  ERROR = 'error',
+  INFO = 'info',
+  WARNING = 'warning',
+}
+
+type NotificationProps = (title: string, message?: React.ReactNode) => void;
+
+type NotificationRemoveProps = (id: number | undefined) => void;
+
+type NotificationTypeFunc = {
+  [key in NotificationTypes]: NotificationProps;
+};
+
+interface NotificationFunc extends NotificationTypeFunc {
+  remove: NotificationRemoveProps;
+}
+
+export const useNotification = (): NotificationFunc => {
+  const { dispatch } = useContext(NotificationContext);
+  // need to move this count somewhere else since it will reset on every new useNotification instance (like switching pages)
+  let notificationCount = 0;


Why don't you move the notification count to the context itself?

Why don't I......

Signed-off-by: Griffin-Sullivan <[email protected]>

ederign · 2024-11-07T15:58:56Z

/lgtm

google-oss-prow · 2024-11-07T15:59:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ederign, lucferbux

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~clients/ui/OWNERS~~ [ederign]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

google-oss-prow bot requested review from alexcreasy, andreyvelich and ederign October 28, 2024 19:29

github-actions bot added the Area/UI label Oct 28, 2024

google-oss-prow bot added the size/L label Oct 28, 2024

Griffin-Sullivan commented Oct 28, 2024

View reviewed changes

clients/ui/frontend/src/components/ToastNotification.tsx Show resolved Hide resolved

Griffin-Sullivan commented Oct 28, 2024

View reviewed changes

Griffin-Sullivan marked this pull request as draft October 28, 2024 19:36

google-oss-prow bot added the do-not-merge/work-in-progress label Oct 28, 2024

Griffin-Sullivan force-pushed the notifications branch from 7c51326 to ec52f09 Compare October 28, 2024 19:51

lucferbux suggested changes Oct 29, 2024

View reviewed changes

Griffin-Sullivan force-pushed the notifications branch 2 times, most recently from 0936ef9 to 61fd6aa Compare October 29, 2024 17:39

Griffin-Sullivan closed this Oct 29, 2024

Griffin-Sullivan reopened this Oct 29, 2024

Griffin-Sullivan force-pushed the notifications branch from 61fd6aa to 7b6e4c2 Compare October 29, 2024 18:26

Griffin-Sullivan marked this pull request as ready for review October 29, 2024 19:21

google-oss-prow bot removed the do-not-merge/work-in-progress label Oct 29, 2024

Griffin-Sullivan force-pushed the notifications branch from 7b6e4c2 to 9227320 Compare November 4, 2024 20:41

google-oss-prow bot added size/XL and removed size/L labels Nov 4, 2024

Griffin-Sullivan commented Nov 4, 2024

View reviewed changes

lucferbux approved these changes Nov 6, 2024

View reviewed changes

Griffin-Sullivan force-pushed the notifications branch from 9227320 to 56a746e Compare November 6, 2024 19:24

Add NotificationContext and useNotification hook

93349aa

Signed-off-by: Griffin-Sullivan <[email protected]>

Griffin-Sullivan force-pushed the notifications branch from 56a746e to 93349aa Compare November 6, 2024 19:26

google-oss-prow bot assigned ederign Nov 7, 2024

google-oss-prow bot added the lgtm label Nov 7, 2024

ederign approved these changes Nov 7, 2024

View reviewed changes

google-oss-prow bot added the approved label Nov 7, 2024

google-oss-prow bot merged commit 962d135 into kubeflow:main Nov 7, 2024
16 checks passed

tarilabs mentioned this pull request Nov 8, 2024

periodic sync upstream KF to midstream ODH opendatahub-io/model-registry#143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NotificationContext and useNotification hook #524

Add NotificationContext and useNotification hook #524

Griffin-Sullivan commented Oct 28, 2024 •

edited

Loading

Griffin-Sullivan Oct 28, 2024

Griffin-Sullivan Oct 28, 2024

lucferbux left a comment

Griffin-Sullivan commented Oct 29, 2024

Griffin-Sullivan commented Nov 4, 2024

Griffin-Sullivan Nov 4, 2024

lucferbux Nov 6, 2024

lucferbux Nov 6, 2024

Griffin-Sullivan Nov 4, 2024

lucferbux Nov 6, 2024

lucferbux left a comment

lucferbux Nov 6, 2024

lucferbux Nov 6, 2024

lucferbux Nov 6, 2024

lucferbux Nov 6, 2024

lucferbux Nov 6, 2024

Griffin-Sullivan Nov 6, 2024

ederign commented Nov 7, 2024

google-oss-prow bot commented Nov 7, 2024

Add NotificationContext and useNotification hook #524

Add NotificationContext and useNotification hook #524

Conversation

Griffin-Sullivan commented Oct 28, 2024 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

Griffin-Sullivan commented Oct 29, 2024

Griffin-Sullivan commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucferbux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ederign commented Nov 7, 2024

google-oss-prow bot commented Nov 7, 2024

Griffin-Sullivan commented Oct 28, 2024 •

edited

Loading