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

Switch to Sonner for toast notifications #9405

Open
wants to merge 8 commits into
base: develop
Choose a base branch
from
Open
39 changes: 22 additions & 17 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,6 @@
"@googlemaps/typescript-guards": "^2.0.3",
"@headlessui/react": "^2.2.0",
"@hello-pangea/dnd": "^17.0.0",
"@pnotify/core": "^5.2.0",
"@pnotify/mobile": "^5.2.0",
"@radix-ui/react-dialog": "^1.1.2",
"@radix-ui/react-dropdown-menu": "^2.1.2",
"@radix-ui/react-icons": "^1.3.2",
Expand Down Expand Up @@ -88,6 +86,7 @@
"i18next": "^23.16.4",
"i18next-browser-languagedetector": "^8.0.0",
"i18next-http-backend": "^3.0.1",
"next-themes": "^0.4.4",
"postcss-loader": "^8.1.1",
"qrcode.react": "^4.1.0",
"raviger": "^4.1.2",
Expand All @@ -99,6 +98,7 @@
"react-infinite-scroll-component": "^6.1.0",
"react-pdf": "^9.1.1",
"react-webcam": "^7.2.0",
"sonner": "^1.7.1",
"tailwind-merge": "^2.5.5",
"tailwindcss-animate": "^1.0.7",
"use-keyboard-shortcut": "^1.1.6",
Expand Down
2 changes: 1 addition & 1 deletion src/App.tsx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just few things...

Ensure notifications are placed in top right of the screen and rich colors are used.

As already mentioned in the issue, use this:

care_fe/src/App.tsx

Lines 38 to 45 in e48d37c

<Toaster
position="top-right"
theme="light"
richColors
// Voluntarily passing empty object as a workaround for `richColors`
// to work. Refer: https://github.com/shadcn-ui/ui/issues/2234.
toastOptions={{}}
/>

Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
import { ReactQueryDevtools } from "@tanstack/react-query-devtools";
import { Suspense } from "react";

import { Toaster } from "@/components/ui/toaster";
import { Toaster } from "@/components/ui/sonner";

import Loading from "@/components/Common/Loading";

Expand Down
9 changes: 2 additions & 7 deletions src/CAREUI/icons/Index.tsx
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
/* eslint-disable i18next/no-literal-string */
import { t } from "i18next";
import React, { useState } from "react";
import { toast } from "sonner";

import CareIcon, { IconName } from "@/CAREUI/icons/CareIcon";
import iconPaths from "@/CAREUI/icons/UniconPaths.json";

import PageTitle from "@/components/Common/PageTitle";

import { useToast } from "@/hooks/useToast";

const IconIndex: React.FC = () => {
const { toast } = useToast();
const [searchTerm, setSearchTerm] = useState("");

const filteredIcons = Object.keys(iconPaths).filter((iconName) =>
Expand All @@ -20,10 +18,7 @@ const IconIndex: React.FC = () => {
const copyToClipboard = (text: string) => {
navigator.clipboard.writeText(text);

toast({
description: "Icon copied to clipboard successfully",
variant: "success",
});
toast.success("Icon copied to clipboard successfully");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add i18n support for the toast message

The success message is hardcoded. Since the project uses i18next (as seen in the imports), this message should be internationalized.

-    toast.success("Icon copied to clipboard successfully");
+    toast.success(t("icon.copy.success"));

Committable suggestion skipped: line range outside the PR's diff.

};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for clipboard operations

The clipboard operation could fail in certain scenarios (permissions, secure context, etc.). Consider adding error handling.

-  const copyToClipboard = (text: string) => {
-    navigator.clipboard.writeText(text);
-    toast.success("Icon copied to clipboard successfully");
+  const copyToClipboard = async (text: string) => {
+    try {
+      await navigator.clipboard.writeText(text);
+      toast.success(t("icon.copy.success"));
+    } catch (error) {
+      toast.error(t("icon.copy.error"));
+      console.error("Failed to copy to clipboard:", error);
+    }
   };
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
const copyToClipboard = (text: string) => {
navigator.clipboard.writeText(text);
toast({
description: "Icon copied to clipboard successfully",
variant: "success",
});
toast.success("Icon copied to clipboard successfully");
};
const copyToClipboard = async (text: string) => {
try {
await navigator.clipboard.writeText(text);
toast.success(t("icon.copy.success"));
} catch (error) {
toast.error(t("icon.copy.error"));
console.error("Failed to copy to clipboard:", error);
}
};


return (
Expand Down
50 changes: 9 additions & 41 deletions src/Utils/Notifications.js
Original file line number Diff line number Diff line change
@@ -1,38 +1,4 @@
import { Stack, alert, defaultModules } from "@pnotify/core";
import * as PNotifyMobile from "@pnotify/mobile";

defaultModules.set(PNotifyMobile, {});

const notifyStack = new Stack({
dir1: "down",
dir2: "left",
firstpos1: 25,
firstpos2: 25,
modal: false,
maxOpen: 3,
maxStrategy: "close",
maxClosureCausesWait: false,
push: "top",
});

const notify = (text, type) => {
const notification = alert({
type: type,
text: text,
styling: "brighttheme",
mode: "light",
sticker: false,
buttons: {
closer: false,
sticker: false,
},
stack: notifyStack,
delay: 3000,
});
notification.refs.elem.addEventListener("click", () => {
notification.close();
});
};
import { toast } from "sonner";

/**
* Formats input string to a more human readable format
Expand Down Expand Up @@ -74,27 +40,29 @@ const notifyError = (error) => {
errorMsg += "\n";
}
}
notify(errorMsg, "error");
toast.error(errorMsg, { duration: 5000 });
};

/** Close all Notifications **/
/** Close all Notifications (Sonner doesn't need this but can be kept for custom implementations) **/
export const closeAllNotifications = () => {
notifyStack.close();
// Sonner doesn't require a close method, but you can manage this with custom logic if needed
// Example: toast.dismiss() could be used to close all toasts if necessary.
toast.dismiss();
};

/** Success message handler */
export const Success = ({ msg }) => {
notify(msg, "success");
toast.success(msg, { duration: 3000 });
};

/** Error message handler */
export const Error = ({ msg }) => {
notify(msg, "error");
toast.error(msg, { duration: 5000 });
};

/** Warning message handler */
export const Warn = ({ msg }) => {
notify(msg, "notice");
toast.info(msg, { duration: 3000 });
};

/** 400 Bad Request handler */
Expand Down
5 changes: 3 additions & 2 deletions src/components/Patient/DailyRounds.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { error } from "@pnotify/core";
import dayjs from "dayjs";
import { navigate } from "raviger";
import { useCallback, useEffect, useState } from "react";
Expand Down Expand Up @@ -339,7 +338,9 @@ export const DailyRounds = (props: any) => {
);

if (investigationError) {
Notification.Error({ msg: error });
Notification.Error({
msg: investigationError.message || investigationError,
});
return;
}
}
Expand Down
30 changes: 30 additions & 0 deletions src/components/ui/sonner.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { useTheme } from "next-themes";
import { Toaster as Sonner } from "sonner";

type ToasterProps = React.ComponentProps<typeof Sonner>;

const Toaster = ({ ...props }: ToasterProps) => {
const { theme = "system" } = useTheme();

return (
<Sonner
theme={theme as ToasterProps["theme"]}
Copy link
Contributor

@Jacobjeevan Jacobjeevan Dec 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add expand prop. Let's have the notifications expanded by default, while the hover state looks nice, I don't think it will work (esp for mobile screens).

And look at tests like Rithvik mentioned above 👍

className="toaster group"
toastOptions={{
classNames: {
toast:
"group toast group-[.toaster]:bg-white group-[.toaster]:text-gray-950 group-[.toaster]:border-gray-200 group-[.toaster]:shadow-lg dark:group-[.toaster]:bg-gray-950 dark:group-[.toaster]:text-gray-50 dark:group-[.toaster]:border-gray-800",
description:
"group-[.toast]:text-gray-500 dark:group-[.toast]:text-gray-400",
actionButton:
"group-[.toast]:bg-gray-900 group-[.toast]:text-gray-50 dark:group-[.toast]:bg-gray-50 dark:group-[.toast]:text-gray-900",
cancelButton:
"group-[.toast]:bg-gray-100 group-[.toast]:text-gray-500 dark:group-[.toast]:bg-gray-800 dark:group-[.toast]:text-gray-400",
},
}}
{...props}
/>
);
};

export { Toaster };
130 changes: 0 additions & 130 deletions src/components/ui/toast.tsx

This file was deleted.

Loading
Loading