Vanessa/2885 workspace side notification by vanessatran-ddi · Pull Request #3371 · GovAlta/ui-components

vanessatran-ddi · 2026-01-28T01:10:51Z

Before (the change)

After (the change)

I also added the docs:

Desktop:

When menu is collapsed:

For mobile:

For any slot content (not only notification case)

Make sure that you've checked the boxes below before you submit the PR

I have read and followed the setup steps
I have created necessary unit tests
I have tested the functionality in both React and Angular.

Steps needed to test

libs/web-components/src/components/badge/Badge.svelte

libs/web-components/src/components/popover/Popover.svelte

libs/web-components/src/components/tabs/Tabs.svelte

bdfranck

Looking great, @vanessatran-ddi! I left a couple comments on some minor things that I noticed.

libs/web-components/src/components/work-side-menu/WorkSideMenu.svelte

libs/web-components/src/components/work-side-menu/WorkSideMenuItem.svelte

vanessatran-ddi · 2026-02-09T22:05:58Z

I press a wrong button

chrisolsen · 2026-02-25T16:57:24Z

libs/web-components/src/components/badge/Badge.svelte

  export let ml: Spacing = null;

+  export let minWidth: string = "";
+  export let justifyContent: string = "";


There could be a type that is created for these css values, both here and in the React/Angular components to make things clearer.

libs/web-components/src/components/popover/Popover.svelte

vanessatran-ddi · 2026-03-17T17:17:19Z

libs/react-components/specs/work-side-menu-with-notification-popover.browser.spec.tsx

+  });
+
+  describe("Desktop viewport", () => {
+    it("should open notification panel in a popover", async () => {


On Firefox this test will fail.

I tested on Firefox manually and here is how notification popover behaves
Firefox version: 145.0.2 (aarch64)
It is related to this issue #3540 (so not related to this PR)

vanessatran-ddi · 2026-03-17T18:44:34Z

libs/react-components/specs/work-side-menu-with-notification-popover.browser.spec.tsx

+  describe("Desktop viewport", () => {
+    // CSS anchor positioning is not supported in Firefox/Safari 18 (see #3540)
+    const isFirefox = navigator.userAgent.includes("Firefox");
+    it.skipIf(isFirefox)("should open notification panel in a popover", async () => {


On Firefox this test will fail.

I tested on Firefox manually and here is how notification popover behaves
Firefox version: 145.0.2 (aarch64) It is related to this issue https://github.com//issues/3540 (so not related to this PR)
I also left a comment on slack

vanessatran-ddi · 2026-03-17T19:08:01Z

libs/web-components/src/components/popover/Popover.svelte

    // (MenuButton, AppHeader, AppHeaderMenu, Dropdown)
    if (_isOpen) {
-      dispatch(_rootEl, "_open");
+      dispatch(_rootEl, "_open", {}, { bubbles: true });


To bubble up to WorkSideMenuItem

libs/web-components/src/components/work-side-menu/WorkSideNotificationPanel.svelte

chrisolsen · 2026-03-17T20:02:49Z

libs/web-components/src/components/work-side-menu/WorkSideNotificationPanel.svelte

+    _updateDateHeadersTimeout = performOnce(
+      _updateDateHeadersTimeout,
+      updateDateHeaders,
+      1,


But is there a reason? 1ms is pretty small and if one event is greater than this it will then fire more than once. The reason why 100 was used was to hopefully guarantee everything works. Was the 99ms noticeably slow?

chrisolsen · 2026-03-17T20:09:40Z

libs/web-components/src/components/link-button/LinkButton.svelte

  export let leadingicon: GoAIconType;
  /** Icon displayed after the button text. */
  export let trailingicon: GoAIconType;
+  export let disabled: string = "false";


Is there a reason this is a string rather than boolean? Now that we can define the props in the svelte:options there is no need to use strings and the toBoolean function.

chrisolsen · 2026-03-17T20:11:03Z

libs/web-components/src/components/link-button/LinkButton.svelte

+  /** Action identifier passed in click events for event delegation patterns. */
+  export let action: string = "";
+  /** Single argument value passed with the action in click events. */
+  export let actionArg: string = "";
+  /** Multiple argument values passed with the action in click events. */
+  export let actionArgs: Record<string, unknown> = {};


Is there a reason the props are being moved around? It is adding to the number of changed lines.

chrisolsen · 2026-03-17T21:45:45Z

libs/web-components/src/components/work-side-menu/WorkSideMenu.svelte

+    _rootEl.removeEventListener("_blurItem", hideTooltip);
    _rootEl.removeEventListener("_toggle", toggleMenu);
+    _rootEl.removeEventListener(
+      "_desktopPopoverOpen",
+      onDesktopPopoverOpen as EventListener,
+    );
+    _rootEl.removeEventListener("_desktopPopoverClose", onDesktopPopoverClose);
+    _rootEl.removeEventListener("keydown", handleMenuKeyDown);


none of the rootEl removeEventListeners are needed as they will be removed when the element is removed from the DOM

chrisolsen · 2026-03-17T21:50:43Z

libs/web-components/src/components/drawer/Drawer.svelte

  /** Sets a data-testid attribute for automated testing. */
  export let testid: string = "drawer";
+  /** Controls visibility of the close button and header. */
+  export let closeButton: "show" | "none" = "show";


This property name is a little vague. closeButtonVisibility would make more sense.

libs/web-components/src/components/tabs/Tabs.svelte

chrisolsen · 2026-03-18T14:44:28Z

libs/react-components/specs/work-side-menu-with-notification-popover.browser.spec.tsx

+        const panel = result.getByTestId("work-space-side-notification-panel");
+        expect(panel).toBeTruthy();


I am pretty sure this has been mentioned before, but the getBy.. functions always return a Locator object, which is always truthy, so there is no need to run the assertion on line 267. These locators should be declared closer to the top of the function if possible. And all locators should not be within a waitFor block otherwise it defeats the purpose of the waitFor.

Hi @chrisolsen I may miss this as this PR was created almost 2 months ago.. I will check on this browser test file again.

chrisolsen · 2026-03-19T00:18:08Z

libs/react-components/specs/work-side-menu-with-notification-popover.browser.spec.tsx

here too and the others in these changes

Let me try again, I thought after markAllBtn.click at line 315, we should safely query after waitFor

The locators don't have to be created when something is visible.

chrisolsen · 2026-03-19T21:44:00Z

libs/web-components/src/components/drawer/Drawer.svelte

  /** Sets a data-testid attribute for automated testing. */
  export let testid: string = "drawer";
+  /** Controls visibility of the close button and header. */
+  export let closeButtonVisibility: "show" | "none" = "show";


For consistency with the other components that have a visibility property, instead of show|none use "visible" | "hidden". I missed this the first time 😞

vanessatran-ddi self-assigned this Jan 28, 2026

vanessatran-ddi marked this pull request as draft January 28, 2026 01:10

vanessatran-ddi force-pushed the vanessa/2885-workspace-side-notification branch from 4f494fb to a47e550 Compare January 28, 2026 21:57