fix: bold the labels, not the values, and give Connection Info's URLs full width
Connection Info rendered every label/value row as a plain label against a bold value. That is backwards for a reference panel: the label is the fixed scaffolding a reader scans down, the value is what differs per connection, and bolding every value means nothing stands out. Reversed across the whole modal — Server Implementation, Discovery and OAuth Details, plus the token captions — rather than in one section, since two halves disagreeing is worse than either convention. Client ID and Auth URL were a Code block in the right half of a two-column grid, so a long value got roughly half the modal width. A CIMD client id IS a URL, so it is long by construction, and it broke mid-token — `…/client-metadata.` / `json` — which reads as a rendering fault rather than as one value. Both now put their label on its own line with the value full width beneath, the way OAuthTokenField already lays out a token, and neither carries an inset surface any more: the grey framed a value that is simply the answer to "Client ID", not a quoted blob needing separation from prose. The transparent treatment is a new `wrapping-plain` theme variant rather than a change to `wrapping`. That one is shared with ElicitationUrlPanel, ListLoadError, MalformedItemsWarning and the CSV fallback, where the inset is doing real work; a test asserts `wrapping` keeps its surface so those consumers cannot be changed by accident. Closes #2328 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GfHQ8vMm5NBgzbUJ6UwxmB Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
fcfc9ba359
commit
82b13a1a12
5 changed files with 168 additions and 22 deletions
|
|
@ -567,6 +567,64 @@ describe("ConnectionInfoContent", () => {
|
|||
expect(screen.getByText("token-123")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("weights labels bold and values normal, in both halves of the modal", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
initializeResult={fullResult}
|
||||
clientCapabilities={fullClientCaps}
|
||||
transport="streamable-http"
|
||||
oauth={{
|
||||
protocol: "standard",
|
||||
authorized: true,
|
||||
clientId: "client-abc",
|
||||
scopes: ["read"],
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
// The convention is the whole point of #2328: the label is the fixed
|
||||
// scaffolding a reader scans down, the value is what differs. Asserted in
|
||||
// Server Implementation *and* OAuth Details, because the defect being
|
||||
// guarded against is the two halves disagreeing — checking one alone would
|
||||
// pass on a modal that is internally inconsistent.
|
||||
for (const label of ["Name", "Protocol", "Client ID", "Scopes"]) {
|
||||
expect(screen.getAllByText(label)[0]).toHaveStyle({ fontWeight: "600" });
|
||||
}
|
||||
for (const value of ["Everything Server", "read"]) {
|
||||
expect(screen.queryByText(value)?.style.fontWeight ?? "").not.toBe("600");
|
||||
}
|
||||
});
|
||||
|
||||
it("gives Client ID and Auth URL the full width, with no inset surface", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
initializeResult={fullResult}
|
||||
clientCapabilities={fullClientCaps}
|
||||
transport="streamable-http"
|
||||
oauth={{
|
||||
protocol: "standard",
|
||||
authorized: true,
|
||||
clientId:
|
||||
"http://127.0.0.1:8093/client-metadata.json?profile=long-enough-to-wrap",
|
||||
authUrl: "https://auth.example.com/authorize",
|
||||
}}
|
||||
/>,
|
||||
);
|
||||
|
||||
// A CIMD client id IS a URL, so it is long by construction. In the
|
||||
// two-column grid it got half the modal and broke mid-token
|
||||
// (`…/client-metadata.` / `json`), which reads as a rendering fault rather
|
||||
// than as one value.
|
||||
for (const value of [
|
||||
"http://127.0.0.1:8093/client-metadata.json?profile=long-enough-to-wrap",
|
||||
"https://auth.example.com/authorize",
|
||||
]) {
|
||||
expect(screen.getByText(value)).toHaveStyle({
|
||||
backgroundColor: "transparent",
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
it("renders EMA idp session when provided", () => {
|
||||
renderWithMantine(
|
||||
<ConnectionInfoContent
|
||||
|
|
|
|||
|
|
@ -79,11 +79,19 @@ export interface ConnectionInfoContentProps {
|
|||
onClearOAuth?: () => void;
|
||||
}
|
||||
|
||||
const ValueText = Text.withProps({
|
||||
// Label/value pairs in this modal read label-bold, value-normal: the label is
|
||||
// the fixed scaffolding a reader scans down, and the value is the thing that
|
||||
// differs per connection. The reverse (which this was until #2328) bolded every
|
||||
// answer, so nothing stood out and the two columns fought each other.
|
||||
const FieldLabel = Text.withProps({
|
||||
size: "sm",
|
||||
fw: 600,
|
||||
});
|
||||
|
||||
const ValueText = Text.withProps({
|
||||
size: "sm",
|
||||
});
|
||||
|
||||
// Shown for Name/Version when the server didn't report `serverInfo` — an em dash
|
||||
// plus an explicit note so the client-side catalog fallback is never mistaken
|
||||
// for a value the server sent. Exported so tests/stories assert against it
|
||||
|
|
@ -104,7 +112,14 @@ const SectionHeading = Title.withProps({
|
|||
// `Code` block — that keeps the whole value visible and removes a scroll region
|
||||
// that would otherwise need its own keyboard access (axe
|
||||
// `scrollable-region-focusable`).
|
||||
const ValueCode = Code.withProps({ variant: "wrapping" });
|
||||
const ValueCode = Code.withProps({ variant: "wrapping-plain" });
|
||||
|
||||
// A long OAuth value (client id, auth URL) gets its label on its own line and
|
||||
// the value across the full modal width beneath it, the way `OAuthTokenField`
|
||||
// already lays out a token. In the two-column grid these values had roughly
|
||||
// half the width and wrapped mid-token — `…/client-metadata.` / `json` — which
|
||||
// reads as a rendering fault rather than as one URL (#2328).
|
||||
const FullWidthField = Stack.withProps({ gap: 4 });
|
||||
|
||||
// One declared sub-option of an extension. Mirrors `CapabilityItem`'s ✓/✗ row
|
||||
// rather than reusing it: that element's `capability` prop is the closed union
|
||||
|
|
@ -306,22 +321,22 @@ export function ConnectionInfoContent({
|
|||
<Stack gap="xs">
|
||||
<SectionHeading>Server Implementation</SectionHeading>
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Name</Text>
|
||||
<FieldLabel>Name</FieldLabel>
|
||||
<ValueText>{displayName}</ValueText>
|
||||
|
||||
<Text size="sm">Version</Text>
|
||||
<FieldLabel>Version</FieldLabel>
|
||||
<ValueText>{displayVersion}</ValueText>
|
||||
|
||||
<Text size="sm">Protocol</Text>
|
||||
<FieldLabel>Protocol</FieldLabel>
|
||||
<ValueText>{protocolVersion || "—"}</ValueText>
|
||||
|
||||
<Text size="sm">Transport</Text>
|
||||
<FieldLabel>Transport</FieldLabel>
|
||||
<Badge variant="outline">{transport}</Badge>
|
||||
|
||||
<Text size="sm">Era</Text>
|
||||
<FieldLabel>Era</FieldLabel>
|
||||
<EraBadge era={protocolEra} />
|
||||
|
||||
<Text size="sm">Session</Text>
|
||||
<FieldLabel>Session</FieldLabel>
|
||||
<ValueText>{formatSession(protocolEra, transport)}</ValueText>
|
||||
</SimpleGrid>
|
||||
</Stack>
|
||||
|
|
@ -330,7 +345,7 @@ export function ConnectionInfoContent({
|
|||
<Stack gap="xs">
|
||||
<SectionHeading>Discovery</SectionHeading>
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Supported versions</Text>
|
||||
<FieldLabel>Supported versions</FieldLabel>
|
||||
<ValueText>
|
||||
{discoverResult.supportedVersions.length > 0
|
||||
? discoverResult.supportedVersions.join(", ")
|
||||
|
|
@ -433,10 +448,10 @@ export function ConnectionInfoContent({
|
|||
<SectionHeading>OAuth Details</SectionHeading>
|
||||
<Stack gap="xs">
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Protocol</Text>
|
||||
<FieldLabel>Protocol</FieldLabel>
|
||||
<ValueText>{formatProtocol(oauth.protocol)}</ValueText>
|
||||
|
||||
<Text size="sm">Status</Text>
|
||||
<FieldLabel>Status</FieldLabel>
|
||||
<Badge
|
||||
variant="outline"
|
||||
color={oauth.authorized ? "green" : "gray"}
|
||||
|
|
@ -445,14 +460,14 @@ export function ConnectionInfoContent({
|
|||
</Badge>
|
||||
</SimpleGrid>
|
||||
{oauth.clientId && (
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Client ID</Text>
|
||||
<FullWidthField>
|
||||
<FieldLabel>Client ID</FieldLabel>
|
||||
<ValueCode>{oauth.clientId}</ValueCode>
|
||||
</SimpleGrid>
|
||||
</FullWidthField>
|
||||
)}
|
||||
{oauth.clientRegistrationKind && (
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Client registration</Text>
|
||||
<FieldLabel>Client registration</FieldLabel>
|
||||
<ValueText>
|
||||
{formatClientRegistrationKind(oauth.clientRegistrationKind)}
|
||||
</ValueText>
|
||||
|
|
@ -460,19 +475,19 @@ export function ConnectionInfoContent({
|
|||
)}
|
||||
{oauth.protocol === "ema" && oauth.idpSession && (
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">IdP session</Text>
|
||||
<FieldLabel>IdP session</FieldLabel>
|
||||
<ValueText>{formatIdpSession(oauth.idpSession)}</ValueText>
|
||||
</SimpleGrid>
|
||||
)}
|
||||
{oauth.authUrl && (
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Auth URL</Text>
|
||||
<FullWidthField>
|
||||
<FieldLabel>Auth URL</FieldLabel>
|
||||
<ValueCode>{oauth.authUrl}</ValueCode>
|
||||
</SimpleGrid>
|
||||
</FullWidthField>
|
||||
)}
|
||||
{oauth.scopes && oauth.scopes.length > 0 && (
|
||||
<SimpleGrid cols={2}>
|
||||
<Text size="sm">Scopes</Text>
|
||||
<FieldLabel>Scopes</FieldLabel>
|
||||
<ValueText>{formatScopes(oauth.scopes)}</ValueText>
|
||||
</SimpleGrid>
|
||||
)}
|
||||
|
|
|
|||
|
|
@ -26,7 +26,10 @@ const CaptionRow = Flex.withProps({
|
|||
wrap: "nowrap",
|
||||
});
|
||||
|
||||
const Caption = Text.withProps({ size: "sm" });
|
||||
// Bold, matching every other field label in Connection Info (#2328) — the
|
||||
// token rows sit in the same OAuth Details list as Client ID and Scopes, so a
|
||||
// lighter caption here would break the column a reader scans down.
|
||||
const Caption = Text.withProps({ size: "sm", fw: 600 });
|
||||
|
||||
const Toolbar = Flex.withProps({
|
||||
gap: 4,
|
||||
|
|
|
|||
60
clients/web/src/theme/Code.test.tsx
Normal file
60
clients/web/src/theme/Code.test.tsx
Normal file
|
|
@ -0,0 +1,60 @@
|
|||
import { describe, it, expect } from "vitest";
|
||||
import { Code } from "@mantine/core";
|
||||
import { renderWithMantine } from "../test/renderWithMantine";
|
||||
|
||||
// `ThemeCode.extend` is applied transitively wherever a Code renders, but no
|
||||
// single component exercises every branch of its styles callback. Same reason
|
||||
// as `Paper.test.tsx` (#1787 brought theme/** under the coverage gate).
|
||||
//
|
||||
// The load-bearing assertion here is the `wrapping` / `wrapping-plain` split
|
||||
// (#2328): `wrapping-plain` exists so Connection Info can drop the inset
|
||||
// surface behind a label/value pair *without* changing `wrapping`, which
|
||||
// ElicitationUrlPanel, ListLoadError, MalformedItemsWarning and the CSV
|
||||
// fallback all rely on to separate a raw value from the prose around it.
|
||||
|
||||
describe("ThemeCode variants", () => {
|
||||
it("renders the default variant", () => {
|
||||
const { getByText } = renderWithMantine(<Code>default</Code>);
|
||||
expect(getByText("default")).toBeTruthy();
|
||||
});
|
||||
|
||||
it("wraps long values in both wrapping variants", () => {
|
||||
for (const variant of ["wrapping", "wrapping-plain"] as const) {
|
||||
const { getByText } = renderWithMantine(
|
||||
<Code variant={variant}>{variant}</Code>,
|
||||
);
|
||||
const style = getByText(variant).style;
|
||||
expect(style.wordBreak).toBe("break-all");
|
||||
expect(style.whiteSpace).toBe("pre-wrap");
|
||||
}
|
||||
});
|
||||
|
||||
it("strips the inset surface for wrapping-plain only", () => {
|
||||
const { getByText: getPlain } = renderWithMantine(
|
||||
<Code variant="wrapping-plain">plain</Code>,
|
||||
);
|
||||
expect(getPlain("plain").style.backgroundColor).toBe("transparent");
|
||||
expect(getPlain("plain").style.padding).toBe("0px");
|
||||
|
||||
// The guard that makes this change safe: `wrapping` keeps its surface, so
|
||||
// every other consumer of it is untouched.
|
||||
const { getByText: getInset } = renderWithMantine(
|
||||
<Code variant="wrapping">inset</Code>,
|
||||
);
|
||||
expect(getInset("inset").style.backgroundColor).not.toBe("transparent");
|
||||
expect(getInset("inset").style.padding).not.toBe("0px");
|
||||
});
|
||||
|
||||
it("clips instead of wrapping for the nowrap variant", () => {
|
||||
const { getByText } = renderWithMantine(<Code variant="nowrap">n</Code>);
|
||||
const style = getByText("n").style;
|
||||
expect(style.whiteSpace).toBe("nowrap");
|
||||
expect(style.overflow).toBe("hidden");
|
||||
expect(style.textOverflow).toBe("ellipsis");
|
||||
});
|
||||
|
||||
it("drops the margin on a block Code", () => {
|
||||
const { getByText } = renderWithMantine(<Code block>b</Code>);
|
||||
expect(getByText("b").style.margin).toBe("0px");
|
||||
});
|
||||
});
|
||||
|
|
@ -10,10 +10,20 @@ export const ThemeCode = Code.extend({
|
|||
backgroundColor:
|
||||
"var(--inspector-code-surface, var(--inspector-surface-code))",
|
||||
};
|
||||
if (props.variant === "wrapping") {
|
||||
if (props.variant === "wrapping" || props.variant === "wrapping-plain") {
|
||||
root.wordBreak = "break-all";
|
||||
root.whiteSpace = "pre-wrap";
|
||||
}
|
||||
if (props.variant === "wrapping-plain") {
|
||||
// Wraps like `wrapping`, but with no inset surface behind it. Used for
|
||||
// label/value rows that read as *values* rather than as quoted blobs —
|
||||
// Connection Info's Client ID and Auth URL. A separate variant rather
|
||||
// than a tweak to `wrapping`, because that one is shared with panels
|
||||
// (elicitation URLs, load errors, CSV fallback) where the grey inset is
|
||||
// doing real work separating a raw value from the prose around it.
|
||||
root.backgroundColor = "transparent";
|
||||
root.padding = "0";
|
||||
}
|
||||
if (props.block) {
|
||||
root.margin = "0";
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue