fix(settings): match notifications type scale to other settings pages (#2447)
Co-authored-by: Kandev Agent <agent@kandev.dev>
This commit is contained in:
@@ -161,9 +161,7 @@ function MobileEventList({
|
||||
key={provider.id}
|
||||
className="flex min-h-11 items-center gap-3 rounded-md border border-muted px-3 py-2"
|
||||
>
|
||||
<span className="min-w-0 flex-1 break-words text-sm font-medium">
|
||||
{provider.name}
|
||||
</span>
|
||||
<span className="min-w-0 flex-1 break-words font-medium">{provider.name}</span>
|
||||
<TestProviderButton provider={provider} onTestProvider={onTestProvider} mobile />
|
||||
<EventCheckbox
|
||||
provider={provider}
|
||||
@@ -197,7 +195,7 @@ function DesktopEventTable({
|
||||
className="hidden overflow-auto rounded-lg border border-muted md:block"
|
||||
data-testid="notification-events-desktop-table"
|
||||
>
|
||||
<table className="min-w-full text-sm">
|
||||
<table className="min-w-full text-xs">
|
||||
<thead className="bg-muted/40">
|
||||
<tr>
|
||||
<th className="px-4 py-3 text-left font-medium">{t("settings:notificationType")}</th>
|
||||
@@ -246,7 +244,7 @@ export function NotificationEventsTable(props: Props) {
|
||||
const { t } = useTranslation();
|
||||
if (props.tableProviders.length === 0) {
|
||||
return (
|
||||
<p className="text-sm text-muted-foreground">{t("settings:noProvidersConfiguredYet")}</p>
|
||||
<p className="text-xs text-muted-foreground">{t("settings:noProvidersConfiguredYet")}</p>
|
||||
);
|
||||
}
|
||||
|
||||
|
||||
@@ -47,8 +47,8 @@ export function DesktopNotificationsSection({
|
||||
<SettingsTarget targetId={GENERAL_SETTINGS_TARGETS.desktopNotifications} className="space-y-4">
|
||||
<div className="flex items-center justify-between gap-4">
|
||||
<div>
|
||||
<div className="text-base font-medium">{t("settings:desktopNotifications")}</div>
|
||||
<p className="text-sm text-muted-foreground">
|
||||
<div className="text-sm font-medium">{t("settings:desktopNotifications")}</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t("settings:desktopNotificationsDescription")}
|
||||
</p>
|
||||
</div>
|
||||
@@ -99,7 +99,7 @@ export function DesktopNotificationsSection({
|
||||
<IconBell className="h-4 w-4" />
|
||||
</Button>
|
||||
</HoverCardTrigger>
|
||||
<HoverCardContent side="top" className="text-sm">
|
||||
<HoverCardContent side="top">
|
||||
{t("settings:notificationsNotShowingHint")}
|
||||
</HoverCardContent>
|
||||
</HoverCard>
|
||||
@@ -107,17 +107,17 @@ export function DesktopNotificationsSection({
|
||||
</div>
|
||||
|
||||
{notificationPermission === "denied" && (
|
||||
<p className="text-sm text-amber-600">
|
||||
<p className="text-xs text-amber-600">
|
||||
{nativeNotifications.isAvailable()
|
||||
? t("settings:notificationsBlockedInOs")
|
||||
: t("settings:notificationsBlockedInBrowser")}
|
||||
</p>
|
||||
)}
|
||||
{notificationPermission === "unsupported" && (
|
||||
<p className="text-sm text-amber-600">{t("settings:notificationsUnsupported")}</p>
|
||||
<p className="text-xs text-amber-600">{t("settings:notificationsUnsupported")}</p>
|
||||
)}
|
||||
{notificationPermission === "error" && (
|
||||
<p className="text-sm text-amber-600">{t("settings:notificationPermissionCheckFailed")}</p>
|
||||
<p className="text-xs text-amber-600">{t("settings:notificationPermissionCheckFailed")}</p>
|
||||
)}
|
||||
</SettingsTarget>
|
||||
);
|
||||
|
||||
@@ -66,8 +66,8 @@ export function NotificationSoundSection({
|
||||
>
|
||||
<div className="flex items-center justify-between gap-4">
|
||||
<div>
|
||||
<div className="text-base font-medium">{t("settings:notificationSound")}</div>
|
||||
<p className="text-sm text-muted-foreground">
|
||||
<div className="text-sm font-medium">{t("settings:notificationSound")}</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t("settings:notificationSoundDescription")}
|
||||
</p>
|
||||
</div>
|
||||
|
||||
@@ -67,4 +67,29 @@ describe("NotificationsSettings", () => {
|
||||
"View installation instructions.",
|
||||
);
|
||||
});
|
||||
|
||||
// The page body renders inside a `SettingsCard`, whose `Card` base is
|
||||
// `text-xs/relaxed`; sibling settings pages keep group headings at `text-sm`
|
||||
// and descriptions/tables at `text-xs`. This body used to hard-code
|
||||
// `text-base`/`text-sm` throughout, so the whole page read a size larger than
|
||||
// its siblings. The `text-2xl` page title above the card is the shared
|
||||
// `SettingsPageTemplate` heading and is deliberately out of scope. jsdom has
|
||||
// no Tailwind, so assert on the utility classes rather than computed sizes.
|
||||
it("keeps the card body within the settings type scale", () => {
|
||||
const { container } = render(
|
||||
<SettingsSaveProvider>
|
||||
<NotificationsSettings />
|
||||
</SettingsSaveProvider>,
|
||||
);
|
||||
|
||||
const body = container.querySelector('[data-slot="card-content"]');
|
||||
expect(body).not.toBeNull();
|
||||
const oversized = [...body!.querySelectorAll("[class]")]
|
||||
.map((element) => element.getAttribute("class") ?? "")
|
||||
// Lookahead, not `(?:\s|$)`: Tailwind's line-height modifier makes the
|
||||
// next character `/` (the Card base is literally `text-xs/relaxed`), so a
|
||||
// consuming boundary would let `text-xl/relaxed` through.
|
||||
.filter((className) => /(?:^|\s)text-(?:base|lg|\d?xl)(?=[\s/]|$)/.test(className));
|
||||
expect(oversized).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -215,13 +215,13 @@ function ExternalProvidersSection({
|
||||
return (
|
||||
<SettingsTarget targetId={GENERAL_SETTINGS_TARGETS.notificationProviders} className="space-y-4">
|
||||
<div>
|
||||
<div className="text-base font-medium">{t("settings:externalProviders")}</div>
|
||||
<p className="text-sm text-muted-foreground">
|
||||
<div className="text-sm font-medium">{t("settings:externalProviders")}</div>
|
||||
<p className="text-xs text-muted-foreground" data-testid="external-providers-description">
|
||||
{t("settings:externalProvidersDescription")}
|
||||
</p>
|
||||
</div>
|
||||
{!appriseAvailable && (
|
||||
<p className="text-sm text-muted-foreground">
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{/* The link is part of the sentence, so the whole notice is one
|
||||
message and the <2> tag addresses the anchor by child index. */}
|
||||
<Trans i18nKey="settings:appriseNotInstalled">
|
||||
@@ -239,7 +239,7 @@ function ExternalProvidersSection({
|
||||
</p>
|
||||
)}
|
||||
{appriseProviders.length === 0 && (
|
||||
<p className="text-sm text-muted-foreground">{t("settings:noAppriseProviders")}</p>
|
||||
<p className="text-xs text-muted-foreground">{t("settings:noAppriseProviders")}</p>
|
||||
)}
|
||||
<AppriseProviderList
|
||||
providers={appriseProviders}
|
||||
@@ -414,8 +414,8 @@ export function NotificationsSettings() {
|
||||
<Separator className="my-4" />
|
||||
<SettingsTarget targetId={GENERAL_SETTINGS_TARGETS.notificationEvents} className="space-y-4">
|
||||
<div>
|
||||
<div className="text-base font-medium">{t("settings:notificationEvents")}</div>
|
||||
<p className="text-sm text-muted-foreground">
|
||||
<div className="text-sm font-medium">{t("settings:notificationEvents")}</div>
|
||||
<p className="text-xs text-muted-foreground">
|
||||
{t("settings:notificationEventsDescription")}
|
||||
</p>
|
||||
</div>
|
||||
@@ -469,7 +469,7 @@ function AppriseProviderForm({
|
||||
data-settings-dirty={formIsDirty}
|
||||
data-settings-dirty-level="container"
|
||||
>
|
||||
<div className="text-base font-medium">{t("settings:appriseProvider")}</div>
|
||||
<div className="text-sm font-medium">{t("settings:appriseProvider")}</div>
|
||||
<Input
|
||||
value={name}
|
||||
onChange={(event) => onNameChange(event.target.value)}
|
||||
|
||||
@@ -0,0 +1,43 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { PROVIDER_NAMES, seedNotificationProviders } from "./notifications-type-scale-helpers";
|
||||
|
||||
test.describe("Mobile notifications settings type scale", () => {
|
||||
test("renders the stacked event list on the shared settings scale", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
prCapture,
|
||||
}) => {
|
||||
await seedNotificationProviders(apiClient);
|
||||
|
||||
// No setViewportSize here on purpose: this spec only runs under the
|
||||
// `mobile-chrome` project (testMatch /mobile-.*\.spec\.ts/, Pixel 5), and
|
||||
// `chromium` testIgnores the same pattern. The testPage context sets only
|
||||
// baseURL, so it inherits the project's device.
|
||||
await testPage.goto("/settings/general/notifications");
|
||||
|
||||
const eventList = testPage.getByTestId("notification-events-mobile-list");
|
||||
await expect(eventList).toBeVisible();
|
||||
|
||||
// One row per provider per event section, so the seeded name repeats. Match
|
||||
// them all rather than .first() — a regression confined to a later section
|
||||
// would otherwise pass.
|
||||
const providerRows = eventList.getByText(PROVIDER_NAMES[0], { exact: true });
|
||||
const rowCount = await providerRows.count();
|
||||
expect(rowCount).toBeGreaterThan(1);
|
||||
|
||||
// Capture before asserting so a run against the pre-fix code still produces
|
||||
// the "before" image for the PR description.
|
||||
await prCapture.screenshot("notifications-mobile", {
|
||||
caption: "Settings → General → Notifications on a phone viewport",
|
||||
});
|
||||
|
||||
// The mobile provider rows used to be text-sm while the section heading
|
||||
// above them inherited the 12px Card base — larger children under a smaller
|
||||
// heading. Every row sits at 12px now.
|
||||
const sizes = await providerRows.evaluateAll((elements) =>
|
||||
elements.map((el) => parseFloat(getComputedStyle(el).fontSize)),
|
||||
);
|
||||
expect(sizes).toHaveLength(rowCount);
|
||||
for (const size of sizes) expect(size).toBeCloseTo(12, 1);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,24 @@
|
||||
import { expect } from "../../fixtures/test-base";
|
||||
import type { ApiClient } from "../../helpers/api-client";
|
||||
|
||||
const TURN_FINISHED = "session.turn_finished";
|
||||
const CLARIFICATION_REQUESTED = "session.clarification_requested";
|
||||
|
||||
/** Two providers so the desktop events table renders more than one column. */
|
||||
export const PROVIDER_NAMES = ["Desktop", "Team channel"];
|
||||
|
||||
/**
|
||||
* The settings type scale is shared by the desktop table and the mobile
|
||||
* stacked list, so both specs seed the same providers. Keeping one helper stops
|
||||
* the two from drifting into asserting against different fixtures.
|
||||
*/
|
||||
export async function seedNotificationProviders(apiClient: ApiClient): Promise<void> {
|
||||
for (const name of PROVIDER_NAMES) {
|
||||
const response = await apiClient.rawRequest("POST", "/api/v1/notification-providers", {
|
||||
name,
|
||||
type: "local",
|
||||
events: [TURN_FINISHED, CLARIFICATION_REQUESTED],
|
||||
});
|
||||
expect(response.ok).toBe(true);
|
||||
}
|
||||
}
|
||||
@@ -0,0 +1,37 @@
|
||||
import { test, expect } from "../../fixtures/test-base";
|
||||
import { seedNotificationProviders } from "./notifications-type-scale-helpers";
|
||||
|
||||
test.describe("Notifications settings type scale", () => {
|
||||
test("renders the card body on the shared settings scale", async ({
|
||||
testPage,
|
||||
apiClient,
|
||||
prCapture,
|
||||
}) => {
|
||||
await seedNotificationProviders(apiClient);
|
||||
|
||||
await testPage.setViewportSize({ width: 1280, height: 1100 });
|
||||
await testPage.goto("/settings/general/notifications");
|
||||
|
||||
const table = testPage.getByTestId("notification-events-desktop-table");
|
||||
await expect(table).toBeVisible();
|
||||
const description = testPage.getByTestId("external-providers-description");
|
||||
await expect(description).toBeVisible();
|
||||
|
||||
// Capture before asserting so a run against the pre-fix code still produces
|
||||
// the "before" image for the PR description.
|
||||
await prCapture.screenshot("notifications-desktop", {
|
||||
caption: "Settings → General → Notifications at 1280px",
|
||||
});
|
||||
|
||||
// The page body inherits the Card base (text-xs/relaxed = 12px); group
|
||||
// headings sit one step up at text-sm (14px). Assert the computed sizes so
|
||||
// the capture is backed by a real measurement, not just an eyeball.
|
||||
expect(
|
||||
await description.evaluate((el) => parseFloat(getComputedStyle(el).fontSize)),
|
||||
).toBeCloseTo(12, 1);
|
||||
expect(await table.evaluate((el) => parseFloat(getComputedStyle(el).fontSize))).toBeCloseTo(
|
||||
12,
|
||||
1,
|
||||
);
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user