Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions src/components/DataTable.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,134 @@
---
/**
* DataTable — semantic shell for the site's comparison tables.
*
* Emits caption → colgroup → thead (th scope="col") → tbody slot. Callers pass only
* <tr> rows whose first cell is a <th scope="row">.
*
* Sets no width, padding, border, border-collapse, table-layout, row height or icon
* size. The callers disagree on all of them — Pricing collapses its borders while the
* Edge matrix separates them by 16px — so appearance stays entirely with the caller.
*
* Styling contract: a caller moving its table shell in here must switch its <style>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the sharpest edge on the new API, and it's enforced only by a doc comment. Someone wiring up a ninth table with a normal scoped <style> gets a table that builds, typechecks, lints clean and renders unstyled — invisible at the call site, which is exactly where the mistake happens.

Two ways out worth weighing: move the shell styling inside DataTable and expose it through custom properties (--dt-cell-padding, --dt-border, …) so callers never need is:global; or keep the contract but make it discoverable, via the component list in CLAUDE.md or a build-time check that a file importing DataTable has no non-global <style>.

Separately, the is:global on this component's own style block (line 110) looks unnecessary: .dt-vh, .dt-caption--hidden and .dt-thead--hidden th are all written in this template, not in callers — I grepped, and no caller writes dt-vh — so a scoped block would match them. If there's a third reason it has to be global, it'd help to record that one instead, since the two given don't appear to apply.

* block to `is:global`, nested under its own class, in the same commit. Astro hashes
* every compound, so a caller's `tbody tr th { … }` stops matching as soon as tbody
* carries this component's hash instead of the caller's.
*/
import type { DataTableColumn } from '@models/data-table';
import { captionId as buildCaptionId } from '@util/caption-id';

interface Props {
/** Emitted as `<caption>`. Required — every table needs an accessible name. */
caption: string;
/** Show the caption. When false it stays in the a11y tree at zero layout cost. */
captionVisible?: boolean;
captionClass?: string;
columns: DataTableColumn[];
/** Class on the `<table>` element — where the caller's CSS attaches. */
class?: string;
/** Emit a `<colgroup>`. Off for callers that size columns on the header cells. */
colgroup?: boolean;
/** Collapse the header row to zero height, e.g. when the visible header sits outside the table. */
headerHidden?: boolean;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Four of the ten props have no caller anywhere in this PR: captionVisible, captionClass, captionId and headerHidden. headerHidden is the telling one — the single table in the codebase that genuinely needs a zero-height header row is Landing/ComparisonTable, and that component doesn't use DataTable at all, so it hand-rolls .comparison-thead th { padding: 0; height: 0; … }. The prop generalised for a use case is unused, and the real use case routes around the component.

Dropping the speculative four until a second caller asks would also cut what a ninth-table author has to read from ten props to six. While you're in here: several props are silently inert in combination — scrollClass does nothing without scrollable, col.width/col.colClass do nothing when colgroup={false} — with no feedback either way.

/** Wrap in a keyboard-scrollable region, named from the caption. */
scrollable?: boolean;
scrollClass?: string;
/** Overrides the derived id. Needed only when one page has two identical captions. */
captionId?: string;
}

const {
caption,
captionVisible = false,
captionClass,
columns,
class: className,
colgroup = true,
headerHidden = false,
scrollable = false,
scrollClass,
captionId,
} = Astro.props;

// `||` not `??`: a caller computing the override from data can hand us an empty
// string, which would emit id="" and leave the scroll region unnamed.
const id = captionId?.trim() || buildCaptionId(caption, 'dt-caption');

// Fragment collapses to nothing, so the table is unwrapped unless `scrollable`.
const Wrapper = scrollable ? 'div' : Fragment;
const wrapperProps = scrollable
? { class: scrollClass, role: 'region', 'aria-labelledby': id, tabindex: '0' }
: {};

/** Renders a header label, turning newlines into line breaks. */
const labelLines = (label: string) => label.split('\n');
---

<Wrapper {...wrapperProps}>
<table class={className}>
<caption id={id} class:list={[captionClass, { 'dt-caption--hidden': !captionVisible }]}>
{captionVisible ? caption : <span class="dt-vh">{caption}</span>}
</caption>
{colgroup && (
<colgroup>
{columns.map((col) => (
<col class={col.colClass} style={col.width ? `width:${col.width}` : undefined} />
))}
</colgroup>
)}
<thead class:list={[{ 'dt-thead--hidden': headerHidden }]}>
<tr>
{columns.map((col) => (
<th scope="col" class={col.thClass} style={col.thStyle}>
{col.label ? (
labelLines(col.label).map((line, i, all) =>
i < all.length - 1 ? (
<>
{line}
<br />
</>
) : (
line
)
)
) : (
<span class="dt-vh">{col.srLabel ?? 'Feature'}</span>
)}
</th>
))}
</tr>
</thead>
<tbody>
<slot />
</tbody>
</table>
</Wrapper>

<style lang="scss" is:global>
@use '../styles/variables' as *;

/* Global because dt-vh is written in callers, on slotted content that carries
their hash. Unlayered so it beats Starlight's layered .sr-only, which is
missing entirely on LegalLayout pages. */
.dt-vh {
@include visually-hidden;
}

/* Both hiders zero the box rather than positioning it: `position: absolute`
blockifies display, stripping the caption role and the columnheader mapping
that scope="col" relies on. */
.dt-caption--hidden {
padding: 0;
height: 0;
line-height: 0;
font-size: 0;
}

.dt-thead--hidden th {
padding: 0;
height: 0;
line-height: 0;
font-size: 0;
border: 0;
}
</style>
69 changes: 69 additions & 0 deletions src/components/DataTableValue.astro
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
---
/**
* DataTableValue — renders one comparison cell's value as text, always.
*
* Icons are decorative (`aria-hidden`); the value itself is a text node, visible when
* the design shows text and visually-hidden when the design shows only an icon. That is
* what makes a cell readable by screen readers and by plain-text extraction.
*
* Never put `aria-label` on an `<Icon>`: astro-icon spreads props onto a bare `<svg>`
* with no `role="img"`, so the name is announced unreliably and extraction sees nothing.
*/
import { Icon } from 'astro-icon/components';

interface Props {
value: boolean | string;
/** Icon for `true`. */
yesIcon?: string;
yesClass?: string;
/** How `false` renders: `'blank'` shows nothing, or give an icon to draw. */
no?: 'blank' | { icon: string; class?: string };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The two branches of the same concept have different shapes: true takes two flat props (yesIcon, yesClass), false takes a string-literal-or-object union. A caller has to remember yesIcon="x" yesClass="y" on one side and no={{ icon: 'x', class: 'y' }} on the other.

Would noIcon?: string; noClass?: string, with "no icon given" meaning blank, be both symmetric and one concept fewer? That also retires the 'blank' sentinel, which currently has to be defaulted, compared (no !== 'blank'), and narrowed before no.icon is reachable.

yesText?: string;
noText?: string;
}

const {
value,
yesIcon = 'tabler:check',
yesClass,
no = 'blank',
yesText = 'Yes',
noText = 'No',
} = Astro.props;

// A blank value would render a blank cell, which is the one thing this component
// exists to prevent. Fail the build rather than ship it: the caller either has a
// real value or wants an explicit one like 'N/A'.
if (typeof value === 'string' && !value.trim()) {
throw new Error(
'DataTableValue: `value` is an empty string, which would render an empty cell. ' +
'Pass a real value, or one of Yes / No / N/A.'
);
}
---

{
value === true ? (
<>
<Icon name={yesIcon} class={yesClass} aria-hidden="true" />
<span class="dt-vh">{yesText}</span>
</>
) : value === false ? (
<>
{no !== 'blank' && <Icon name={no.icon} class={no.class} aria-hidden="true" />}
<span class="dt-vh">{noText}</span>
</>
) : (
value
)
}

<style lang="scss" is:global>
@use '../styles/variables' as *;

// Declared here as well as in DataTable: this component can be used inside a
// hand-written table, and without the rule the value text would be visible.
.dt-vh {
@include visually-hidden;
}
</style>
23 changes: 21 additions & 2 deletions src/components/IotHub/SectionTable.astro
Original file line number Diff line number Diff line change
@@ -1,19 +1,27 @@
---
import { captionId as buildCaptionId } from '@util/caption-id';

interface Props {
heading: string;
intro?: string;
columns: string[];
rows: string[][];
/** Overrides the derived id. Needed only when one page has two identical headings. */
captionId?: string;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This escape hatch is copy-pasted from DataTable (same doc comment) and, like that one, has no caller — CategoryInfoSections passes only heading/intro/columns/rows. It's also where the contract is least reasonable to satisfy: headings come from data and several SectionTables render on one page, so honouring it requires the parent to notice a collision and thread an override down, and nothing prompts it to.

Since the parent is the only component that can see the sibling headings, deriving the id there (index-suffixed) would be simpler than exposing an override nobody sets.

}

const { heading, intro, columns, rows } = Astro.props;
const { heading, intro, columns, rows, captionId } = Astro.props;

const id = captionId?.trim() || buildCaptionId(heading, 'ih-table');
---

<section class="ih-table">
<h2 class="ih-table__title">{heading}</h2>
{intro && <p class="ih-table__intro">{intro}</p>}
<div class="ih-table__scroll">
<div class="ih-table__scroll" role="region" aria-labelledby={id} tabindex="0">

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabindex="0" here is unconditional, so every SectionTable becomes a keyboard tab stop announced as a region — including the ones that never overflow, which on wide viewports is most of them. The ARIA pattern this comes from wants the tab stop only when the region actually scrolls; applied unconditionally it adds a stop-and-announce to the tab order on every IoT Hub category page for no benefit.

Same applies to the new scrollable region in Pricing/ComparisonTable and the .table-scroll in ce-vs-pe-diff. If you want to keep it simple, a tiny shared script that sets tabindex only when scrollWidth > clientWidth (and on resize) would give the right behaviour in all three places.

<table class="ih-table__table">
{/* Hidden: the <h2> above already shows this text. */}
<caption id={id} class="ih-table__caption">{heading}</caption>
<thead>
<tr>
{columns.map((col) => <th scope="col">{col}</th>)}
Expand Down Expand Up @@ -52,6 +60,17 @@ const { heading, intro, columns, rows } = Astro.props;
@include iot-hub-section-intro;
}

// Zeroed rather than positioned: `position: absolute` would blockify the
// caption and strip the table's accessible name.
.ih-table__caption {
padding: 0;
height: 0;
line-height: 0;
font-size: 0;
overflow: hidden;
color: transparent;
}

.ih-table__scroll {
overflow-x: auto;
// Rounded outer frame (matches buttons/cards); cells draw only inner
Expand Down
Loading