diff --git a/static/app/components/core/entityHeader/entityHeader.mdx b/static/app/components/core/entityHeader/entityHeader.mdx index 8b35894f7c98..34d821136abc 100644 --- a/static/app/components/core/entityHeader/entityHeader.mdx +++ b/static/app/components/core/entityHeader/entityHeader.mdx @@ -46,9 +46,12 @@ Only `title` is required. The grid template is built from whichever other slots stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 0}, - {type: 'text', label: 'Rage Clicks', value: 2}, - {type: 'link', label: 'Errors', value: 1, to: '/replays/1/?t_main=errors'}, + {label: 'Dead Clicks', value: 0}, + {label: 'Rage Clicks', value: 2}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: 1, + }, ], }} metadata={{ @@ -92,9 +95,12 @@ Only `title` is required. The grid template is built from whichever other slots stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 0}, - {type: 'text', label: 'Rage Clicks', value: 2}, - {type: 'link', label: 'Errors', value: 1, to: '/replays/1/?t_main=errors'}, + {label: 'Dead Clicks', value: 0}, + {label: 'Rage Clicks', value: 2}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: 1, + }, ], }} metadata={{ @@ -127,8 +133,8 @@ Only `title` is required. The grid template is built from whichever other slots ```jsx stats={{label: 'Replay stats', items: [ - isVideoReplay ? null : {type: 'text', label: t('Dead Clicks'), value: deadClicks}, - {type: 'text', label: t('Errors'), value: errors.length}, + isVideoReplay ? null : {label: t('Dead Clicks'), value: deadClicks}, + {label: t('Errors'), value: errors.length}, ]}} ``` @@ -150,8 +156,8 @@ A declared slot holds its space before its contents arrive. Passing `metadata` r stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 0}, - {type: 'text', label: 'Errors', value: 1}, + {label: 'Dead Clicks', value: 0}, + {label: 'Errors', value: 1}, ], }} metadata={{ @@ -176,8 +182,8 @@ A declared slot holds its space before its contents arrive. Passing `metadata` r stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 0}, - {type: 'text', label: 'Errors', value: 1}, + {label: 'Dead Clicks', value: 0}, + {label: 'Errors', value: 1}, ], }} metadata={{ @@ -199,7 +205,7 @@ Slots that resolve on their own schedule carry their own flag, additive to the h ```jsx stats={{label: 'Replay stats', items: [ - {type: 'text', label: t('Events'), value: count, isLoading: isCountPending}, + {label: t('Events'), value: count, isLoading: isCountPending}, ]}} ``` @@ -243,7 +249,7 @@ A destination outside the product takes `externalHref` in place of `to`, such as A link title is accent-coloured and carries no underline, the same treatment a stat or metadata link gets — one link treatment across the header. Colour is doing the work here, so reserve `link` for a destination the title genuinely implies; a title that merely _could_ link is better left as text with the destination offered elsewhere. -Note where the link sits in each of the three. A stat's **label** navigates, because a link named by a bare count says nothing. A metadata item's **value** navigates, because the value is where the user is going. A title navigates **whole** — it is the entity, so there is no half to choose between. +Note where the link sits in each of the three. A stat chooses **per half** — its label, its value, or both — because the two can lead to different places: the measure, or the things it counts. A metadata item's **value** navigates, because the value is where the user is going. A title navigates **whole** — it is the entity, so there is no half to choose between. The title is still an `h2`, now with a link inside it, which is how most of the web marks up an article title. A screen reader announces both — "Replay user, anonymous@example.com, heading level 2, link" — and the link carries the same name as the heading, so a links list reads that rather than a bare address. @@ -326,7 +332,7 @@ tags={[isLive ? {t('Live')} : null]} A stat is one measurement: a `label` naming it and a `value`. Both are rendered, on a shared baseline. ```jsx -stats={{label: 'Replay stats', items: [{type: 'text', label: t('Rage Clicks'), value: 2}]}} +stats={{label: 'Replay stats', items: [{label: t('Rage Clicks'), value: 2}]}} ``` Keep the value to the single number the label names. Anything that explains it belongs in a tooltip, not in the row — a stat that renders its own detail inline fights the density the row is built for. @@ -339,28 +345,37 @@ The label comes first, on screen and in the DOM: "Errors 3", not "3 Errors". A s Each half is read as itself — "Errors", then "3" — rather than composed into one announcement. Nothing is hidden and nothing is renamed, except the dash shown for an empty value (see No data, below), so what a screen reader reads is what is on screen. -A `link` stat's anchor wraps the label only, and is named by it: a links list reads "Errors", which says where it leads. The count is not part of the name and does not need to be — it sits in the same list item, which is context enough for the link's purpose. +A link is named by the text it wraps. A label link reads "Errors" in a links list; a value link reads "7". Neither is renamed to fold in the other half — the other half sits in the same list item, which is context enough for the link's purpose, and what a screen reader reads stays what is on screen. ### Types -A stat declares whether it is `text` or a `link`. That is not only documentation. A stat whose type depended on its data would render a different element once that data arrived, and swapping a span for an anchor mid-load moves the row. So a stat that links, links from the first render — including when its count is still zero. +Each half — `label` and `value` — declares whether it is `text` or a `link`. That is not only documentation. A half whose type depended on its data would render a different element once that data arrived, and swapping a span for an anchor mid-load moves the row. So a half that links, links from the first render — including when its count is still zero. + +A plain string for `label`, or a number, string or `null` for `value`, is shorthand for a `text` half. The object form is `{type, content, tooltip?}`, and a `link` half adds where it leads: `to` inside the product, or `externalHref` outside it, plus an optional `onClick` for analytics. ```jsx -{type: 'text', label: t('Rage Clicks'), value: 2} -{type: 'link', label: t('Errors'), value: errors.length, to: errorsTab} +{label: t('Rage Clicks'), value: 2} +{label: {type: 'link', content: t('Dead Clicks'), to: breadcrumbsTab}, value: deadClicks} +{label: t('Errors'), value: {type: 'link', content: errors.length, to: errorsUrl}} ``` -The value may change as often as it likes. The type may not. +The content may change as often as it likes. The type may not. + +### Which half links + +Link the **label** when the destination is about what the stat measures — the tab where dead clicks are listed, the docs that explain an install size. Link the **value** when the destination is the things the value counts — the errors behind an error count. -On a `link` stat it is the **label** that navigates, not the value: a link named by its own digits says nothing in a screen reader's links list. +Both halves can link, to the same place or to different ones. Two links to the same place cost a keyboard user a second tab stop for nothing, so prefer one unless both destinations earn their place. ### Tooltips -`labelTooltip` covers a label that is jargon on its own. `valueTooltip` carries a breakdown — a replay's errors can come from several projects, so the stat shows the total and the tooltip splits it by project. Both take structured content, so either can be built from `Tooltip.Header`, `Tooltip.Grid` and `Tooltip.Row`. +A tooltip belongs to the half it explains, as that half's `tooltip`. On the label it covers a label that is jargon on its own. On the value it carries a breakdown — a replay's errors can come from several projects, so the stat shows the total and the tooltip splits it by project. Either takes structured content, so it can be built from `Tooltip.Header`, `Tooltip.Grid` and `Tooltip.Row`. -Each tooltip has one trigger, on its own half, and either or both can be set on any stat type. Tab visits the label, then the value, and each opens only its own tooltip. On a `link` stat the label tooltip attaches to the link itself rather than nesting a second tab stop inside it, and the value tooltip is a separate trigger after the link. +A tooltip needs the object form, even on a `text` half: shorthand has nowhere to put one. -Colour says it navigates; an underline says it explains. A link label is accent-coloured, a tooltipped label is underlined, and one that does both is both — as the three stats below are, in order. A single treatment would mean a link that only navigates looked like one that explains, and an underline reads as "hover for more" to most people, so that is the meaning it keeps. +Each tooltip has one trigger, on its own half. Tab visits the label, then the value, and each opens only its own tooltip. On a `link` half the tooltip attaches to the link itself rather than nesting a second tab stop inside it. + +Colour says it navigates; an underline says it explains. A link is accent-coloured, a tooltipped half is underlined, and one that does both is both — as the stats below show. A single treatment would mean a link that only navigates looked like one that explains, and an underline reads as "hover for more" to most people, so that is the meaning it keeps. @@ -452,16 +496,19 @@ Screen readers cannot reach the reason yet. Focus on the value reads "No data", ```jsx // ❌ Read aloud as "Download Size", "dash" -{type: 'text', label: t('Download Size'), value: '-'} +{label: t('Download Size'), value: '-'} // ✅ Read as "Download Size", "No data", with the reason on the value { - type: 'text', label: t('Download Size'), - value: null, - valueTooltip: t('Download size is not reported for this platform.'), + value: { + type: 'text', + content: null, + tooltip: t('Download size is not reported for this platform.'), + }, } -{type: 'link', label: t('Errors'), value: null, to: errorsUrl} +{label: {type: 'link', content: t('Errors'), to: errorsTab}, value: null} +{label: t('Issues'), value: {type: 'link', content: null, to: issuesUrl}} ``` ## People @@ -505,7 +552,7 @@ Two values rather than one joined string is the point: a browser is its name _an With one value, `label` is its key — "Started at". With two, it is the parent of both, and `secondary.label` names the child: "version" under "Browser" composes to "Browser version", which is what the row announces and what the qualifier shows on hover. -A value can navigate. `type: 'link'` renders it as one, named from the key and the value together so a links list reads "Release 8.42.0" rather than a bare version. Note the mirror of a stat, where the _label_ links instead: there a link named by a bare count says nothing, while here the value is where the user is going. +A value can navigate. `type: 'link'` renders it as one, named from the key and the value together so a links list reads "Release 8.42.0" rather than a bare version. Unlike a stat, where both halves are on screen and each link is named by its own text, a metadata key is usually hidden — so the link has to carry it. ```jsx { diff --git a/static/app/components/core/entityHeader/entityHeader.snapshots.tsx b/static/app/components/core/entityHeader/entityHeader.snapshots.tsx index afe440921803..eb91eae753af 100644 --- a/static/app/components/core/entityHeader/entityHeader.snapshots.tsx +++ b/static/app/components/core/entityHeader/entityHeader.snapshots.tsx @@ -35,9 +35,9 @@ const props: EntityHeaderProps = { stats: { label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 0}, - {type: 'text', label: 'Rage Clicks', value: 2}, - {type: 'text', label: 'Errors', value: 1}, + {label: 'Dead Clicks', value: 0}, + {label: 'Rage Clicks', value: 2}, + {label: 'Errors', value: 1}, ], }, }; @@ -75,8 +75,22 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'link', label: 'Errors', value: 3, to: '/replays/1/'}, - {type: 'text', label: 'Errors', value: 3}, + {label: {type: 'link', content: 'Errors', to: '/replays/1/'}, value: 3}, + {label: 'Errors', value: {type: 'link', content: 3, to: '/replays/1/'}}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/'}, + value: {type: 'link', content: 3, to: '/issues/'}, + }, + { + label: { + type: 'link', + content: 'Errors', + to: '/replays/1/', + tooltip: 'Why', + }, + value: {type: 'link', content: 3, to: '/issues/', tooltip: 'Split'}, + }, + {label: 'Errors', value: 3}, ], }} /> @@ -96,14 +110,20 @@ describe('EntityHeader', () => { stats={{ label: 'Build stats', items: [ - {type: 'text', label: 'Install Size', value: '12 MB'}, - {type: 'text', label: 'Cost', value: null}, - {type: 'link', label: 'Errors', value: null, to: '/replays/1/'}, + {label: 'Install Size', value: '12 MB'}, + {label: 'Cost', value: null}, + {label: {type: 'link', content: 'Errors', to: '/replays/1/'}, value: null}, + { + label: 'Issues', + value: {type: 'link', content: null, to: '/issues/'}, + }, { - type: 'text', label: 'Download Size', - value: null, - valueTooltip: 'Not reported for this platform', + value: { + type: 'text', + content: null, + tooltip: 'Not reported for this platform', + }, }, ], }} @@ -129,7 +149,7 @@ describe('EntityHeader', () => { }} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 0}], + items: [{label: 'Errors', value: 0}], }} /> ), diff --git a/static/app/components/core/entityHeader/entityHeader.spec.tsx b/static/app/components/core/entityHeader/entityHeader.spec.tsx index fd2b5e1cebd9..bafd1d954b80 100644 --- a/static/app/components/core/entityHeader/entityHeader.spec.tsx +++ b/static/app/components/core/entityHeader/entityHeader.spec.tsx @@ -10,7 +10,7 @@ import { } from 'sentry-test/reactTestingLibrary'; import {Tag} from '@sentry/scraps/badge'; -import type {EntityHeaderProps} from '@sentry/scraps/entityHeader'; +import type {EntityHeaderProps, EntityHeaderStatProps} from '@sentry/scraps/entityHeader'; import {EntityHeader} from '@sentry/scraps/entityHeader'; import {Tooltip} from '@sentry/scraps/tooltip'; @@ -218,7 +218,7 @@ describe('EntityHeader', () => { }); describe('stats and metadata', () => { - it('names a stat link by where it leads, not by its number', () => { + it('names a label link by its label, leaving the count beside it', () => { render( { label: 'Replay stats', items: [ { - type: 'link', - label: 'Dead Clicks', + label: { + type: 'link', + content: 'Dead Clicks', + to: '/replays/1/?t_main=breadcrumbs', + }, value: 4, - to: '/replays/1/?t_main=breadcrumbs', }, - {type: 'text', label: 'Rage Clicks', value: 0}, + {label: 'Rage Clicks', value: 0}, ], }} /> @@ -259,17 +261,18 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Text', value: 1}, - {type: 'link', label: 'Link', value: 2, to: '/a/'}, - {type: 'text', label: 'Label tip', value: 3, labelTooltip: 'About'}, - {type: 'text', label: 'Value tip', value: 4, valueTooltip: 'Split'}, + {label: 'Text', value: 1}, + {label: {type: 'link', content: 'Link', to: '/a/'}, value: 2}, + {label: {type: 'text', content: 'Label tip', tooltip: 'About'}, value: 3}, + {label: 'Value tip', value: {type: 'text', content: 4, tooltip: 'Split'}}, { - type: 'link', - label: 'Both tips', - value: 5, - to: '/b/', - labelTooltip: 'About', - valueTooltip: 'Split', + label: {type: 'link', content: 'Both tips', to: '/b/', tooltip: 'About'}, + value: {type: 'text', content: 5, tooltip: 'Split'}, + }, + {label: 'Value link', value: {type: 'link', content: 6, to: '/c/'}}, + { + label: {type: 'link', content: 'Both links', to: '/d/'}, + value: {type: 'link', content: 7, to: '/d/'}, }, ], }} @@ -285,6 +288,8 @@ describe('EntityHeader', () => { expect(items[2]).toHaveTextContent(/^Label tip3$/); expect(items[3]).toHaveTextContent(/^Value tip4$/); expect(items[4]).toHaveTextContent(/^Both tips5$/); + expect(items[5]).toHaveTextContent(/^Value link6$/); + expect(items[6]).toHaveTextContent(/^Both links7$/); }); it('visits each tooltip in turn, label first, each opening its own', async () => { @@ -295,11 +300,12 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'text', - label: 'Errors', - value: 3, - labelTooltip: 'Errors recorded during this replay', - valueTooltip: 'From 2 projects', + label: { + type: 'text', + content: 'Errors', + tooltip: 'Errors recorded during this replay', + }, + value: {type: 'text', content: 3, tooltip: 'From 2 projects'}, }, ], }} @@ -326,11 +332,8 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'link', - label: 'Errors', - value: 3, - to: '/replays/1/?t_main=errors', - valueTooltip: 'From 2 projects', + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: {type: 'text', content: 3, tooltip: 'From 2 projects'}, }, ], }} @@ -356,10 +359,12 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'text', - label: 'Dead Clicks', + label: { + type: 'text', + content: 'Dead Clicks', + tooltip: 'A click that did not change anything.', + }, value: 4, - labelTooltip: 'A click that did not change anything.', }, ], }} @@ -380,22 +385,24 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'text', label: 'Errors', - value: 3, - valueTooltip: ( - - Errors - - 2}> - javascript - - 1}> - python - - - - ), + value: { + type: 'text', + content: 3, + tooltip: ( + + Errors + + 2}> + javascript + + 1}> + python + + + + ), + }, }, ], }} @@ -418,11 +425,13 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'link', - label: 'Errors', + label: { + type: 'link', + content: 'Errors', + to: '/replays/1/?t_main=errors', + tooltip: 'Errors recorded during this replay', + }, value: 3, - to: '/replays/1/?t_main=errors', - labelTooltip: 'Errors recorded during this replay', }, ], }} @@ -453,7 +462,10 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'link', label: 'Errors', value: 0, to: '/replays/1/?t_main=errors'}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: 0, + }, ], }} /> @@ -467,7 +479,10 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'link', label: 'Errors', value: 3, to: '/replays/1/?t_main=errors'}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: 3, + }, ], }} /> @@ -477,6 +492,210 @@ describe('EntityHeader', () => { expect(screen.getByRole('listitem')).toHaveTextContent(/^Errors3$/); }); + it('names a value link by its own text, after the label', () => { + render( + + ); + + // Nothing is renamed: the link reads "7", and the label beside it in the + // same list item says what it counts. + const link = screen.getByRole('link', {name: '7'}); + expect(link).toHaveAttribute('href', '/explore/traces/?query=errors'); + expect(link).not.toHaveAttribute('aria-label'); + expect(screen.queryByRole('link', {name: 'Errors'})).not.toBeInTheDocument(); + expect(screen.getByRole('listitem')).toHaveTextContent(/^Errors7$/); + }); + + it('attaches a value tooltip to the value link rather than nesting a tab stop', async () => { + render( + + ); + + const link = screen.getByRole('link', {name: '7'}); + + await userEvent.tab(); + expect(link).toHaveFocus(); + expect(await screen.findByText('From 2 tools')).toBeInTheDocument(); + + await userEvent.tab(); + expect(document.body).toHaveFocus(); + }); + + it('lets both halves link, visiting the label first', async () => { + render( + + ); + + const label = screen.getByRole('link', {name: 'Errors'}); + const value = screen.getByRole('link', {name: '3'}); + expect(label).toHaveAttribute('href', '/replays/1/?t_main=errors'); + expect(value).toHaveAttribute('href', '/issues/?query=replay'); + expect(label).not.toContainElement(value); + + await userEvent.tab(); + expect(label).toHaveFocus(); + await userEvent.tab(); + expect(value).toHaveFocus(); + }); + + it('opens an external stat link in a new tab, on either half', () => { + render( + + ); + + const label = screen.getByRole('link', {name: 'Install Size'}); + expect(label).toHaveAttribute('href', 'https://docs.sentry.io/size/'); + expect(label).toHaveAttribute('target', '_blank'); + + const value = screen.getByRole('link', {name: '12 MB'}); + expect(value).toHaveAttribute('href', 'https://github.com/getsentry/sentry/pull/1'); + expect(value).toHaveAttribute('target', '_blank'); + }); + + it('calls each half its own click handler', async () => { + const onLabelClick = jest.fn(); + const onValueClick = jest.fn(); + + render( + + ); + + await userEvent.click(screen.getByRole('link', {name: 'Errors'})); + expect(onLabelClick).toHaveBeenCalledTimes(1); + expect(onValueClick).not.toHaveBeenCalled(); + + await userEvent.click(screen.getByRole('link', {name: '3'})); + expect(onValueClick).toHaveBeenCalledTimes(1); + expect(onLabelClick).toHaveBeenCalledTimes(1); + }); + + it('calls the click handler of an external link half', async () => { + const onClick = jest.fn(); + + render( + + ); + + await userEvent.click(screen.getByRole('link', {name: '12 MB'})); + expect(onClick).toHaveBeenCalledTimes(1); + }); + + it('keeps the same element when a value link receives its value', () => { + const build = (content: number): EntityHeaderProps => ({ + title: {type: 'text', label: 'Replay user', value: 'Session'}, + stats: { + label: 'Replay stats', + items: [ + { + label: 'Errors', + value: {type: 'link', content, to: '/issues/?query=replay'}, + }, + ], + }, + }); + + const {rerender} = render(); + const before = screen.getByRole('link', {name: '0'}); + + rerender(); + + expect(screen.getByRole('link', {name: '3'})).toBe(before); + }); + it('drops null entries so callers can inline conditionals', () => { const isVideoReplay = true; render( @@ -485,10 +704,8 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - isVideoReplay - ? null - : {type: 'text' as const, label: 'Dead Clicks', value: 4}, - {type: 'text', label: 'Errors', value: 2}, + isVideoReplay ? null : {label: 'Dead Clicks', value: 4}, + {label: 'Errors', value: 2}, ], }} metadata={{ @@ -517,8 +734,8 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - showViewers ? {type: 'text' as const, label: 'Seen By', value: 2} : null, - {type: 'text', label: 'Note', value: 7}, + showViewers ? {label: 'Seen By', value: 2} : null, + {label: 'Note', value: 7}, ], }} /> @@ -547,7 +764,7 @@ describe('EntityHeader', () => { people={{users: [], isLoading: true, label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -560,7 +777,7 @@ describe('EntityHeader', () => { people={{users, label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -630,7 +847,7 @@ describe('EntityHeader', () => { }} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -655,7 +872,7 @@ describe('EntityHeader', () => { people={{users: [UserFixture({id: '1', name: 'Alice'})], label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -684,7 +901,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Replay user', value: 'Session'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -700,7 +917,7 @@ describe('EntityHeader', () => { people={{users: [], label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -723,7 +940,7 @@ describe('EntityHeader', () => { ); // A links list showing "8.42.0" says nothing about what it leads to. - // The mirror of a stat, where the label links and carries the count. + // Unlike a stat, the key is hidden, so the link has to carry it. expect(screen.getByRole('link', {name: 'Version 8.42.0'})).toHaveAttribute( 'href', '/releases/8.42.0/' @@ -854,47 +1071,66 @@ describe('EntityHeader', () => { expect(item).toHaveTextContent(/^BrowserChromeBrowser version144\.0\.0$/); }); - it.each([ + it.each<[string, EntityHeaderStatProps]>([ [ - 'a link stat with a label tooltip', - {type: 'link', to: '/e/', labelTooltip: 'Why'}, + 'a link label with a tooltip', + {label: {type: 'link', content: 'Errors', to: '/e/', tooltip: 'Why'}, value: 3}, ], [ - 'a link stat with a value tooltip', - {type: 'link', to: '/e/', valueTooltip: 'Split'}, + 'a link label with a value tooltip', + { + label: {type: 'link', content: 'Errors', to: '/e/'}, + value: {type: 'text', content: 3, tooltip: 'Split'}, + }, + ], + [ + 'a link value with a tooltip', + {label: 'Errors', value: {type: 'link', content: 3, to: '/e/', tooltip: 'Split'}}, ], [ - 'a link stat with both tooltips', - {type: 'link', to: '/e/', labelTooltip: 'Why', valueTooltip: 'Split'}, + 'a link label and a link value, both with tooltips', + { + label: {type: 'link', content: 'Errors', to: '/e/', tooltip: 'Why'}, + value: {type: 'link', content: 3, to: '/e/', tooltip: 'Split'}, + }, ], [ - 'a text stat with both tooltips', - {type: 'text', labelTooltip: 'Why', valueTooltip: 'Split'}, + 'a text label and a text value, both with tooltips', + { + label: {type: 'text', content: 'Errors', tooltip: 'Why'}, + value: {type: 'text', content: 3, tooltip: 'Split'}, + }, ], [ - 'an empty link stat with a label tooltip', - {type: 'link', to: '/e/', value: null, labelTooltip: 'Why'}, + 'an empty value under a link label with a tooltip', + { + label: {type: 'link', content: 'Errors', to: '/e/', tooltip: 'Why'}, + value: null, + }, ], [ - 'an empty link stat with a value tooltip', - {type: 'link', to: '/e/', value: null, valueTooltip: 'Not recorded'}, + 'an empty link value with a tooltip', + { + label: 'Errors', + value: {type: 'link', content: null, to: '/e/', tooltip: 'Not recorded'}, + }, ], [ - 'an empty text stat with a value tooltip', - {type: 'text', value: null, valueTooltip: 'Not recorded'}, + 'an empty text value with a tooltip', + {label: 'Errors', value: {type: 'text', content: null, tooltip: 'Not recorded'}}, ], [ - 'an empty text stat with both tooltips', - {type: 'text', value: null, labelTooltip: 'Why', valueTooltip: 'Not recorded'}, + 'an empty text value, both with tooltips', + { + label: {type: 'text', content: 'Errors', tooltip: 'Why'}, + value: {type: 'text', content: null, tooltip: 'Not recorded'}, + }, ], - ] as const)('never hides a focusable element on %s', (_, stat) => { + ])('never hides a focusable element on %s', (_, stat) => { render( ); @@ -915,7 +1151,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Build', value: '1.0.0'}} stats={{ label: 'Build stats', - items: [{type: 'text', label: 'Cost', value: null}], + items: [{label: 'Cost', value: null}], }} /> ); @@ -940,10 +1176,12 @@ describe('EntityHeader', () => { label: 'Replay stats', items: [ { - type: 'link', - label: 'Errors', + label: { + type: 'link', + content: 'Errors', + to: '/replays/1/?t_main=errors', + }, value: null, - to: '/replays/1/?t_main=errors', }, ], }} @@ -960,6 +1198,30 @@ describe('EntityHeader', () => { ); }); + it('keeps an empty link value a link, read as no data', () => { + render( + + ); + + // Named by its own text, the same as any value link — the dash is + // hidden and "No data" is read in its place. + const link = screen.getByRole('link', {name: 'No data'}); + expect(link).toHaveAttribute('href', '/replays/1/?t_main=errors'); + expect(within(link).getByText('—')).toHaveAttribute('aria-hidden', 'true'); + expect(screen.getByRole('listitem')).toHaveTextContent(/^Errors—No data$/); + }); + it('explains why there is no value from the value trigger', async () => { render( { label: 'Build stats', items: [ { - type: 'text', label: 'Cost', - value: null, - valueTooltip: 'No cost is recorded for local builds', + value: { + type: 'text', + content: null, + tooltip: 'No cost is recorded for local builds', + }, }, ], }} @@ -997,10 +1261,12 @@ describe('EntityHeader', () => { label: 'Build stats', items: [ { - type: 'text', - label: 'Install Size', + label: { + type: 'text', + content: 'Install Size', + tooltip: 'Size on device', + }, value: null, - labelTooltip: 'Size on device', }, ], }} @@ -1021,7 +1287,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Replay user', value: 'Session'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 0}], + items: [{label: 'Errors', value: 0}], }} /> ); @@ -1039,7 +1305,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Build', value: '1.0.0'}} stats={{ label: 'Build stats', - items: [{type: 'text', label: 'Cost', value: null, isLoading: true}], + items: [{label: 'Cost', value: null, isLoading: true}], }} /> ); @@ -1063,8 +1329,8 @@ describe('EntityHeader', () => { stats={{ label: 'Build stats', items: [ - {type: 'text', label: 'Placeholder Size', value: '-'}, - {type: 'text', label: 'Real Size', value: '12 MB'}, + {label: 'Placeholder Size', value: '-'}, + {label: 'Real Size', value: '12 MB'}, ], }} /> @@ -1082,8 +1348,8 @@ describe('EntityHeader', () => { stats={{ label: 'Build stats', items: [ - {type: 'text', label: 'Placeholder Size', value: '-'}, - {type: 'text', label: 'Real Size', value: '13 MB'}, + {label: 'Placeholder Size', value: '-'}, + {label: 'Real Size', value: '13 MB'}, ], }} /> @@ -1097,7 +1363,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Build', value: '1.0.0'}} stats={{ label: 'Build stats', - items: [{type: 'text', label: 'Padded Size', value: ' — '}], + items: [{label: 'Padded Size', value: ' — '}], }} /> ); @@ -1142,8 +1408,8 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 4}, - {type: 'text', label: 'Errors', value: 2}, + {label: 'Dead Clicks', value: 4}, + {label: 'Errors', value: 2}, ], }} metadata={{ @@ -1188,7 +1454,7 @@ describe('EntityHeader', () => { }, stats: { label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 0}], + items: [{label: 'Errors', value: 0}], }, }); @@ -1211,7 +1477,7 @@ describe('EntityHeader', () => { people={{users: [], label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -1224,7 +1490,7 @@ describe('EntityHeader', () => { people={{users: [], isLoading: true, label: 'Viewed by'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -1240,8 +1506,8 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'text', label: 'Dead Clicks', value: 4}, - {type: 'text', label: 'Errors', value: 0, isLoading: true}, + {label: 'Dead Clicks', value: 4}, + {label: 'Errors', value: 0, isLoading: true}, ], }} /> @@ -1260,7 +1526,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Replay user', value: 'Session'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 0, isLoading: true}], + items: [{label: 'Errors', value: 0, isLoading: true}], }} /> ); @@ -1272,7 +1538,7 @@ describe('EntityHeader', () => { title={{type: 'text', label: 'Replay user', value: 'Session'}} stats={{ label: 'Replay stats', - items: [{type: 'text', label: 'Errors', value: 2}], + items: [{label: 'Errors', value: 2}], }} /> ); @@ -1289,7 +1555,10 @@ describe('EntityHeader', () => { stats={{ label: 'Replay stats', items: [ - {type: 'link', label: 'Errors', value: 3, to: '/replays/1/?t_main=errors'}, + { + label: {type: 'link', content: 'Errors', to: '/replays/1/?t_main=errors'}, + value: 3, + }, ], }} metadata={{ diff --git a/static/app/components/core/entityHeader/items/entityHeaderStat.tsx b/static/app/components/core/entityHeader/items/entityHeaderStat.tsx index a81ed2743cea..b71d5c1c0364 100644 --- a/static/app/components/core/entityHeader/items/entityHeaderStat.tsx +++ b/static/app/components/core/entityHeader/items/entityHeaderStat.tsx @@ -4,8 +4,8 @@ import {VisuallyHidden} from '@react-aria/visually-hidden'; import {ROW_HEIGHT, STAT_VALUE_HEIGHT} from '@sentry/scraps/entityHeader/constants'; import {InfoText} from '@sentry/scraps/info'; import {Flex} from '@sentry/scraps/layout'; -import type {LinkProps} from '@sentry/scraps/link'; -import {Link} from '@sentry/scraps/link'; +import type {LinkDestination} from '@sentry/scraps/link'; +import {ExternalLink, Link} from '@sentry/scraps/link'; import {Text} from '@sentry/scraps/text'; import {Tooltip} from '@sentry/scraps/tooltip'; import {useTranslation} from '@sentry/scraps/translation/useTranslation'; @@ -18,26 +18,57 @@ import {Placeholder} from 'sentry/components/placeholder'; */ type EntityHeaderStatValue = number | string | null; -interface EntityHeaderStatBase { +/** + * One half of a stat — its label or its value. Each half declares what it is, + * rather than the component inferring it from whether a destination happens + * to be defined. The content may change as often as it likes; the type may + * not, so a link half stays a link when its count is zero or empty. + */ +type EntityHeaderStatPart = { + content: TContent; + /** + * Takes structured content as readily as a string, so it can be built from + * `Tooltip.Header`, `Tooltip.Grid` and `Tooltip.Row`. Keep it readable + * without links: nothing inside a tooltip can be reached by keyboard. + */ + tooltip?: React.ReactNode; +} & ( + | {type: 'text'} + | ({ + type: 'link'; + onClick?: () => void; + } & LinkDestination) +); + +export interface EntityHeaderStatProps { /** - * Short, static label such as "Dead Clicks". + * Short, static label such as "Dead Clicks". A string is shorthand for a + * `text` label. + * + * Link the label when the destination is about what the stat measures — + * the tab where dead clicks are listed. Its tooltip explains a label that is + * jargon on its own. */ - label: string; + label: string | EntityHeaderStatPart; /** * The measurement itself. Text, not a node: the row is built for single - * numbers, and any breakdown belongs in `valueTooltip`. + * numbers, and any breakdown belongs in the value's `tooltip`. A number, + * string or `null` is shorthand for a `text` value. + * + * Link the value when the destination is the things it counts — the errors + * behind an error count. Its tooltip carries what the value is made of, or, + * when the content is `null`, why there is none. * - * `null` means the stat applies to this entity but there is nothing to - * show — no cost recorded, no size reported. It renders a muted "—" and is - * read "No data". Never pass a placeholder such as `'-'` or `'—'` yourself: - * a screen reader reads it aloud as the data ("dash"). + * `null` means the data was never collected for this entity — no cost + * recorded, no size reported. It renders a "—" and is read "No data". Never + * pass a placeholder such as `'-'` or `'—'` yourself: a screen reader reads it + * aloud as the data ("dash"). * * Not for a real zero (pass `0`), a value still loading (pass `isLoading`), * or a stat that does not apply to this entity (pass `null` for the whole - * item instead). An empty link stat is still a link, named by its label, - * since its type must not change once the data loads. + * item instead). */ - value: EntityHeaderStatValue; + value: EntityHeaderStatValue | EntityHeaderStatPart; /** * Hold this one stat as a skeleton while the rest of the header is live. Use * it when the value comes from a request that settles after the entity does; @@ -46,40 +77,8 @@ interface EntityHeaderStatBase { * `isLoading`. */ isLoading?: boolean; - /** - * What the stat measures, for a label that is jargon on its own. Takes - * structured content as readily as a string, so it can be built from - * `Tooltip.Header`, `Tooltip.Grid` and `Tooltip.Row`. - */ - labelTooltip?: React.ReactNode; - /** - * What the value is made of — the projects behind an error count, say — - * or, when `value` is `null`, why there is none. Takes structured content, - * which is how a breakdown stays out of the row. - * - * Keep a reason readable without links: nothing inside a tooltip can be - * reached by keyboard. - */ - valueTooltip?: React.ReactNode; } -/** - * A stat declares what it is, rather than the component inferring it from - * whether a destination happens to be defined. The value may change as often - * as it likes; the type may not. - */ -export type EntityHeaderStatProps = - | ({type: 'text'} & EntityHeaderStatBase) - | ({ - /** - * Where the label leads. Required, so the stat cannot quietly stop being - * a link when its count is zero. - */ - to: LinkProps['to']; - type: 'link'; - onClick?: () => void; - } & EntityHeaderStatBase); - const VALUE_STYLES = {size: 'lg', bold: true, tabular: true, wrap: 'nowrap'} as const; const LABEL_STYLES = { @@ -113,6 +112,20 @@ function warnOnPlaceholderValue(label: string, value: EntityHeaderStatValue): vo ); } +function getLabelPart( + label: EntityHeaderStatProps['label'] +): EntityHeaderStatPart { + return typeof label === 'string' ? {type: 'text', content: label} : label; +} + +function getValuePart( + value: EntityHeaderStatProps['value'] +): EntityHeaderStatPart { + return value !== null && typeof value === 'object' + ? value + : {type: 'text', content: value}; +} + function EmptyValue() { const {t} = useTranslation(); // Translators: read by a screen reader in place of the dash shown for a stat that has nothing to show, e.g. no cost recorded @@ -128,47 +141,41 @@ function EmptyValue() { ); } -function StatValue({value, valueTooltip}: EntityHeaderStatProps) { - // An empty value keeps the value's typography, so the row's height and - // baseline do not move, and is muted so it does not read as a measurement. - const isEmpty = value === null; - const content = isEmpty ? : value; - const variant = isEmpty ? 'muted' : undefined; - - if (valueTooltip) { +function StatPart({ + part, + children, + styles, + textVariant, +}: { + children: React.ReactNode; + part: EntityHeaderStatPart; + styles: typeof LABEL_STYLES | typeof VALUE_STYLES; + textVariant?: 'muted'; +}) { + if (part.type === 'link') { + // Named by its own text. The other half is read in the same list item, + // which is context enough for the link's purpose. return ( - - {content} - - ); - } - - return ( - - {content} - - ); -} - -function StatLabel(props: EntityHeaderStatProps) { - const {label, labelTooltip} = props; - - if (props.type === 'link') { - const {to, onClick} = props; - // Named by its label alone — where it leads. The count beside it is read - // in the same list item, which is context enough for the link's purpose. - return ( - + {styleProps => { - const link = ( - - {label} - - ); + const link = + part.externalHref === undefined ? ( + + {children} + + ) : ( + + {children} + + ); // The tooltip attaches to the link rather than wrapping it in // InfoText, which would put a second tab stop inside the anchor. - return labelTooltip ? ( - + return part.tooltip ? ( + {link} ) : ( @@ -179,17 +186,17 @@ function StatLabel(props: EntityHeaderStatProps) { ); } - if (labelTooltip) { + if (part.tooltip) { return ( - - {label} + + {children} ); } return ( - - {label} + + {children} ); } @@ -203,7 +210,11 @@ export function EntityHeaderStat(props: EntityHeaderStatProps) { ); } - warnOnPlaceholderValue(props.label, props.value); + const label = getLabelPart(props.label); + const value = getValuePart(props.value); + const isEmpty = value.content === null; + + warnOnPlaceholderValue(label.content, value.content); return ( @@ -213,8 +224,21 @@ export function EntityHeaderStat(props: EntityHeaderStatProps) { then "3". */} - - + + {label.content} + + {/* + An empty value keeps the value's typography, so the row's height and + baseline do not move, and is muted so it does not read as a + measurement — unless it links, where the colour says it navigates. + */} + + {isEmpty ? : value.content} + ); diff --git a/static/app/views/explore/replays/detail/header/replayDetailsEntityHeader.tsx b/static/app/views/explore/replays/detail/header/replayDetailsEntityHeader.tsx index a4bd4a22d681..bd63bf0c9ba4 100644 --- a/static/app/views/explore/replays/detail/header/replayDetailsEntityHeader.tsx +++ b/static/app/views/explore/replays/detail/header/replayDetailsEntityHeader.tsx @@ -134,30 +134,27 @@ export function ReplayDetailsEntityHeader({readerResult}: Props) { items: [ showDeadRageClicks ? { - type: 'link', - label: t('Dead Clicks'), + label: {type: 'link', content: t('Dead Clicks'), to: breadcrumbTab}, value: deadClicks, - to: breadcrumbTab, isLoading: statsLoading, } : null, showDeadRageClicks ? { - type: 'link', - label: t('Rage Clicks'), + label: {type: 'link', content: t('Rage Clicks'), to: breadcrumbTab}, value: rageClicks, - to: breadcrumbTab, isLoading: statsLoading, } : null, { - type: 'link', - label: t('Errors'), - value: nonFeedbackErrors.length, - to: errorsTab, - valueTooltip: nonFeedbackErrors.length ? ( - - ) : undefined, + label: {type: 'link', content: t('Errors'), to: errorsTab}, + value: { + type: 'text', + content: nonFeedbackErrors.length, + tooltip: nonFeedbackErrors.length ? ( + + ) : undefined, + }, isLoading: statsLoading, }, ], diff --git a/static/app/views/preprod/buildComparison/header/buildCompareHeaderContent.tsx b/static/app/views/preprod/buildComparison/header/buildCompareHeaderContent.tsx index 198ba3eda7c1..517c0b56c416 100644 --- a/static/app/views/preprod/buildComparison/header/buildCompareHeaderContent.tsx +++ b/static/app/views/preprod/buildComparison/header/buildCompareHeaderContent.tsx @@ -120,16 +120,20 @@ export function BuildCompareHeaderContent(props: BuildCompareHeaderContentProps) label: t('Build size'), items: [ { - type: 'text', - label: installStat.label, - labelTooltip: installStat.tooltip, + label: { + type: 'text', + content: installStat.label, + tooltip: installStat.tooltip, + }, value: formattedPrimaryMetricInstallSize(sizeInfo), isLoading, }, { - type: 'text', - label: t('Bytes Transferred'), - labelTooltip: t('Over the network'), + label: { + type: 'text', + content: t('Bytes Transferred'), + tooltip: t('Over the network'), + }, value: formattedPrimaryMetricDownloadSize(sizeInfo), isLoading, },