Merge pull request #219 from Coffey-Labs/fix/unsaved-dialog-emphasis

Highlight saving, not discarding, on the unsaved-changes guard

web/src/lib/__tests__/unsavedChanges.test.tsx

@@ -0,0 +1,110 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import { act } from "react";
+import { createRoot, type Root } from "react-dom/client";
+
+/*
+ * The guard's answers, and which of them the dialog leans on.
+ *
+ * It shipped with "Discard changes" as the only choice carrying a colour, which
+ * made losing the work the easy thing to click on a dialog whose entire purpose
+ * is to stop that (#175). The emphasis belongs on the safe answer; the
+ * destructive one stays legible as destructive without being the loudest thing
+ * in the box.
+ */
+
+const choiceDialog = vi.fn();
+vi.mock("@/ui/dialog", () => ({ choiceDialog: (...args: unknown[]) => choiceDialog(...args) }));
+
+const { confirmLeaveUnsaved, useUnsavedChanges } = await import("@/lib/unsavedChanges");
+
+interface Choice { value: string; label: string; hint?: string; danger?: boolean; primary?: boolean }
+
+const save = vi.fn(async () => true);
+const discard = vi.fn();
+
+function Editor() {
+  useUnsavedChanges({ dirty: true, save, discard, message: "Your filters have unsaved changes." });
+  return null;
+}
+
+let root: Root | null = null;
+let host: HTMLDivElement | null = null;
+
+/** One dirty editor on screen, which is what registers anything at all. */
+function mountDirtyEditor() {
+  host = document.createElement("div");
+  document.body.appendChild(host);
+  root = createRoot(host);
+  act(() => root!.render(<Editor />));
+}
+
+const asked = () => choiceDialog.mock.calls[0]![0] as { choices: Choice[]; cancelLabel: string; title: string; message: string };
+
+beforeEach(() => {
+  (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+  choiceDialog.mockReset().mockResolvedValue(null);
+  save.mockClear().mockResolvedValue(true);
+  discard.mockClear();
+  mountDirtyEditor();
+});
+
+afterEach(() => {
+  act(() => root!.unmount());
+  host!.remove();
+  root = null;
+  host = null;
+});
+
+describe("the unsaved-changes guard", () => {
+  it("highlights saving, not discarding", async () => {
+    await confirmLeaveUnsaved();
+    const [saveChoice, discardChoice] = asked().choices;
+    expect(saveChoice!.primary).toBe(true);
+    expect(discardChoice!.primary).toBeFalsy();
+  });
+
+  it("still says which answer is the destructive one", async () => {
+    await confirmLeaveUnsaved();
+    const [saveChoice, discardChoice] = asked().choices;
+    expect(discardChoice!.danger).toBe(true);
+    expect(discardChoice!.hint).toBeTruthy();
+    expect(saveChoice!.danger).toBeFalsy();
+  });
+
+  it("offers saving first, and staying as the way out", async () => {
+    await confirmLeaveUnsaved();
+    expect(asked().choices.map((c) => c.value)).toEqual(["save", "discard"]);
+    expect(asked().cancelLabel).toBeTruthy();
+  });
+
+  it("says what is about to be lost, in the editor's own words", async () => {
+    await confirmLeaveUnsaved();
+    expect(asked().message).toBe("Your filters have unsaved changes.");
+  });
+
+  it("stays put when the dialog is dismissed, and loses nothing", async () => {
+    choiceDialog.mockResolvedValue(null);
+    await expect(confirmLeaveUnsaved()).resolves.toBe(false);
+    expect(save).not.toHaveBeenCalled();
+    expect(discard).not.toHaveBeenCalled();
+  });
+
+  it("saves and goes when saving is chosen", async () => {
+    choiceDialog.mockResolvedValue("save");
+    await expect(confirmLeaveUnsaved()).resolves.toBe(true);
+    expect(save).toHaveBeenCalledOnce();
+  });
+
+  it("holds you on the page when the save fails", async () => {
+    choiceDialog.mockResolvedValue("save");
+    save.mockResolvedValue(false);
+    await expect(confirmLeaveUnsaved()).resolves.toBe(false);
+  });
+
+  it("discards and goes when discarding is chosen", async () => {
+    choiceDialog.mockResolvedValue("discard");
+    await expect(confirmLeaveUnsaved()).resolves.toBe(true);
+    expect(discard).toHaveBeenCalledOnce();
+    expect(save).not.toHaveBeenCalled();
+  });
+});

web/src/lib/unsavedChanges.ts

@@ -91,7 +91,11 @@ export async function confirmLeaveUnsaved(): Promise<boolean> {
       title: t("Save your changes?"),
       message: pending.message,
       choices: [
-        { value: "save", label: t("Save changes") },
+        // Save is the highlighted one. Highlighting the destructive answer makes
+        // losing the work the easy thing to click, which is the failure this
+        // dialog exists to prevent -- #175, after the guard shipped with the
+        // emphasis the wrong way round.
+        { value: "save", label: t("Save changes"), primary: true },
         { value: "discard", label: t("Discard changes"), hint: t("What you changed here will be lost."), danger: true },
       ],
       cancelLabel: t("Stay here"),

web/src/styles/app.css

@@ -715,6 +715,10 @@ a.menu-item:hover { color: var(--fg); }
 .dialog-choices { display: flex; flex-direction: column; gap: 8px; }
 .dialog-choice { display: flex; flex-direction: column; align-items: flex-start; gap: 2px; width: 100%; text-align: left; padding: 10px 12px; height: auto; }
 .dialog-choice small { font-weight: 400; opacity: 0.75; }
+/* Destructive, the way .menu-item.danger is: a red label on the ordinary
+   surface. A filled red button in a list of answers is the loudest thing in the
+   dialog, which is wrong when it is the answer that loses your work. */
+.dialog-choice.danger { color: var(--danger); }
 
 /* Toasts ----------------------------------------------------------------- */
 .toast-host { position: fixed; left: 50%; bottom: 24px; transform: translateX(-50%); z-index: 3000; display: flex; flex-direction: column; gap: 8px; align-items: center; pointer-events: none; padding: 0 12px; width: 100%; max-width: 520px; }

web/src/ui/dialog.tsx

@@ -93,6 +93,17 @@ export interface DialogChoice {
   /** Shown under the label, for the choice that needs the caveat. */
   hint?: string;
   danger?: boolean;
+  /**
+   * The safe answer, given the weight a dialog's confirm button has.
+   *
+   * A list of choices has no default until one is said to be, and the
+   * destructive one must not become it by being the only thing with a colour --
+   * which is what "Discard changes" was, on a guard whose whole purpose is to
+   * stop you losing work ([#175]).
+   *
+   * [#175]: https://github.com/Coffey-Labs/ihasmail/issues/175
+   */
+  primary?: boolean;
 }
 
 interface ConfirmRequest {
@@ -178,7 +189,7 @@ export function ConfirmHost() {
       {req.kind === "choice" && (
         <div className="dialog-choices">
           {req.choices?.map((c) => (
-            <button key={c.value} className={`btn dialog-choice ${c.danger ? "btn-danger" : ""}`} onClick={() => done(c.value)}>
+            <button key={c.value} className={`btn dialog-choice ${c.primary ? "btn-primary" : ""} ${c.danger ? "danger" : ""}`} onClick={() => done(c.value)}>
               <span>{c.label}</span>
               {c.hint && <small>{c.hint}</small>}
             </button>