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

Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5c0f052
Switch to Sonner for toast notifications #9394
nilay-v3rma Dec 13, 2024
bb44c6a
toaster position and theme updates
nilay-v3rma Dec 19, 2024
a828352
updated cypress tests to check for sonner toast notifications
nilay-v3rma Dec 19, 2024
9434c27
updated closeNotification to close sonner toast notification
nilay-v3rma Dec 19, 2024
0761bc7
Merge branch 'develop' into issues/9394/use-sonner-notifications
nilay-v3rma Dec 19, 2024
c3b3b8d
Update package-lock.json
rithviknishad Dec 19, 2024
cbd8d11
Merge branch 'develop' into issues/9394/use-sonner-notifications
nihal467 Dec 20, 2024
05702ac
Merge remote-tracking branch 'origin' into issues/9394/use-sonner-not…
rithviknishad Dec 23, 2024
63c3a83
fix-verify notification cypress command
nilay-v3rma Dec 24, 2024
fc2efc2
Merge branch 'develop' into issues/9394/use-sonner-notifications
nilay-v3rma Dec 24, 2024
7007e04
fix-close notification cypress command
nilay-v3rma Dec 26, 2024
e1d536f
update lockfile
rithviknishad Dec 28, 2024
0ba432f
fix asset create
rithviknishad Dec 28, 2024
bfbcd97
Fixes #9559:No Notice Available Icon Misaligned in Mobile View (#9562)
pranavchaitu Dec 25, 2024
603b1b8
Bump @yudiel/react-qr-scanner from 2.0.8 to 2.1.0 (#9566)
dependabot[bot] Dec 25, 2024
7754bbd
Bump @sentry/browser from 8.45.1 to 8.47.0 (#9565)
dependabot[bot] Dec 25, 2024
86d171b
fixed flaky test in doctor notes (#9569)
nihal467 Dec 26, 2024
94a1d53
Always show "online" for authenticated user profiles (#9574)
abhimanyurajeesh Dec 26, 2024
7a0874e
Bump @radix-ui/react-popover from 1.1.2 to 1.1.4 (#9576)
dependabot[bot] Dec 27, 2024
809900e
Bump react-i18next from 15.1.3 to 15.2.0 (#9575)
dependabot[bot] Dec 27, 2024
9a085cf
Fix: the selected medicine issue in the placeholder (#9596)
AdityaJ2305 Dec 28, 2024
0177534
Removed unwanted check in the patient discharge test (#9601)
nihal467 Dec 28, 2024
de2ff2e
Merge branch 'develop' into issues/9394/use-sonner-notifications
nilay-v3rma Dec 28, 2024
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
7 changes: 5 additions & 2 deletions cypress/pageobject/Facility/FacilityManage.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,10 +51,13 @@ class FacilityManage {
text: string | RegExp,
isRegex = false,
) {
// Selector for the Sonner toast content
const toastSelector = "li[data-sonner-toast] div[data-title]";

if (isRegex) {
cy.get(".pnotify-text").should("be.visible").contains(text);
cy.get(toastSelector).should("be.visible").contains(text);
} else {
cy.get(".pnotify-text").should("be.visible").and("contain.text", text);
cy.get(toastSelector).should("be.visible").and("contain.text", text);
Jacobjeevan marked this conversation as resolved.
Show resolved Hide resolved
}
}

Expand Down
9 changes: 7 additions & 2 deletions cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,13 @@ Cypress.Commands.add(
},
);

//check sonner toast notification
Cypress.Commands.add("verifyNotification", (text) => {
return cy.get(".pnotify-container").should("exist").contains(text);
return cy
.get("li[data-sonner-toast] div[data-title]")
.should("exist")
.and("be.visible")
.contains(text);
rithviknishad marked this conversation as resolved.
Show resolved Hide resolved
});

Cypress.Commands.add("clearAllFilters", () => {
Expand Down Expand Up @@ -221,7 +226,7 @@ Cypress.Commands.add("preventPrint", () => {
});

Cypress.Commands.add("closeNotification", () => {
cy.get(".pnotify")
cy.get("li[data-sonner-toast] div[data-title]")
rithviknishad marked this conversation as resolved.
Show resolved Hide resolved
.should("exist")
.each(($div) => {
cy.wrap($div).click();
Expand Down
43 changes: 24 additions & 19 deletions package-lock.json

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

6 changes: 3 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,7 @@
"@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.4",
"@radix-ui/react-dialog": "^1.1.2",
"@radix-ui/react-dropdown-menu": "^2.1.2",
"@radix-ui/react-icons": "^1.3.2",
"@radix-ui/react-label": "^2.1.0",
Expand Down Expand Up @@ -88,6 +86,7 @@
"i18next": "^23.16.4",
"i18next-browser-languagedetector": "^8.0.2",
"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
11 changes: 9 additions & 2 deletions src/App.tsx
rithviknishad marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import {
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 Expand Up @@ -54,7 +54,14 @@ const App = () => {
<Integrations.Sentry disabled={!import.meta.env.PROD} />
<Integrations.Plausible />
</HistoryAPIProvider>
<Toaster />
<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={{}}
/>
</PluginEngine>
</PubSubProvider>
</Suspense>
Expand Down
19 changes: 9 additions & 10 deletions src/CAREUI/icons/Index.tsx
Original file line number Diff line number Diff line change
@@ -1,29 +1,28 @@
/* 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) =>
iconName.toLowerCase().includes(searchTerm.toLowerCase()),
);

const copyToClipboard = (text: string) => {
navigator.clipboard.writeText(text);

toast({
description: "Icon copied to clipboard successfully",
variant: "success",
});
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";
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

⚠️ Potential z-index conflict with Sonner toast notifications

Based on the analysis of z-index values in the codebase and Sonner's configuration:

  • Found z-index values up to 100000 in src/style/index.css
  • Sonner's Toaster component is configured in App.tsx but lacks explicit z-index configuration
  • By default, Sonner uses a z-index of 9999, which is lower than some existing z-index values (100000) in the codebase

To resolve potential overlay issues:

  • Add a zIndex prop to the Toaster component with a value higher than 100000
<Toaster
  position="top-right"
  theme="light"
  richColors
  zIndex={100001}
/>
🔗 Analysis chain

LGTM! Verify z-index configuration with Sonner.

The import change looks good. However, based on the PR comments about toast notifications being overlaid by other elements, ensure Sonner's z-index configuration is properly set.


Let me check the Sonner toast configuration in the codebase to ensure proper z-index setup.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any custom z-index configurations that might conflict with Sonner
rg -g '*.{css,scss,tsx,jsx}' 'z-index:'

Length of output: 755


Script:

#!/bin/bash
# Search for Sonner Toaster component configuration
rg -g '*.{tsx,jsx,js}' -A 5 '<Toaster'

Length of output: 382


/**
* 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 });
};
Comment on lines 56 to 58
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

Rename Error function to avoid shadowing global Error.

The function name 'Error' shadows the built-in Error object. Consider renaming it to be more specific.

-export const Error = ({ msg }) => {
+export const ShowError = ({ msg }) => {
   toast.error(msg, { duration: 5000 });
 };
📝 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
export const Error = ({ msg }) => {
notify(msg, "error");
toast.error(msg, { duration: 5000 });
};
export const ShowError = ({ msg }) => {
toast.error(msg, { duration: 5000 });
};
🧰 Tools
🪛 Biome (1.9.4)

[error] 56-56: Do not shadow the global "Error" property.

Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.

(lint/suspicious/noShadowRestrictedNames)


/** 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 };
Loading