Make the comparison tables semantic and leave one main per page - #631
Make the comparison tables semantic and leave one main per page#631rusikv wants to merge 18 commits into
Conversation
Pricing/ComparisonTable and Landing/ComparisonTable both claimed .comparison-table and .comparison-title. Pricing declares its block is:global, so its table rules (width, border-collapse, table-layout, min-width) would apply to Landing's <section> as soon as both render on one page. Renames Pricing's 19 classes to pc-comparison-*, including the four selectors in pricing/index.astro and PlanFeatureItem that reach into them. No markup or style changes.
Emits caption, colgroup, thead with th scope="col", and a tbody slot; callers supply rows whose first cell is a th scope="row". Sets no box metrics at all, since the existing table skins agree on none of them. DataTableValue renders a cell's value as text in every case, with the icon marked decorative. That is what makes a cell readable both by a screen reader and by plain-text extraction, which currently sees the icon-only cells as empty. Ships its own visually-hidden class rather than reusing Starlight's: that one is layered, so any unlayered rule beats it, and it is absent from LegalLayout pages entirely. Both the hidden caption and the hidden header row zero their box instead of being positioned, because position: absolute would blockify them and strip the caption role and the columnheader mapping that scope="col" depends on.
These four already emit a real <table>; they were missing an accessible name, column and row header semantics, and a keyboard-reachable scroll region. Content is untouched. Two cells needed care rather than a straight td-to-th swap: UseCase/ComparisonTable styled th with the dark header row's white text, so a body row header would have rendered white on a light background. Scoped that rule to thead th and extended the td rule to tbody th. development-services had a 1279px media query keyed on td alone, so the converted row header would have lost its wider padding between 960 and 1279px. Extended that selector too. The hardware spec table gets a caption and scope="row" but no thead: it is a key/value table with no column headers to name. LegalLayout had the opposite defect from the rest of this work — no main landmark at all on its ten-plus pages, since they do not route through Starlight. Its container is now a main. Adds a visually-hidden mixin so the recipe has one home.
Replaces the hand-written table shell with the shared primitive, which brings a hidden caption, th scope="col" on every column including the first, and a keyboard-reachable scroll region. Cell values are now text in every case. The check icon becomes decorative with a hidden "Yes" beside it, and the empty span that stood for "not included" becomes a hidden "No" — the cell stays visually blank, which is the intended design. Plain-text extraction of this table goes from 45 empty cells to none. The feature column moves from td to th scope="row" and gains an explicit text-align, since th would otherwise centre it. The skin needed no changes: its block is already is:global, so the rules rooted at .pc-comparison-table still reach the thead and tbody now emitted by DataTable.
Removes the self-closing <td /> that produced the site's only W3C "Stray end tag td" error, on /products/paas/. It becomes a real th scope="col" carrying hidden text, which also gives the first column a header and the row a complete set of column headers. Value icons lose their aria-label — unreliable on a bare svg with no role="img", and invisible to text extraction — in favour of a decorative icon beside hidden text. Extraction of this table goes from 39 empty cells to none. The style block had to become is:global in the same commit as the markup move: Astro hashes every compound, so a rule such as "thead tr th" would have silently stopped matching once the shell came from DataTable. Every rule is nested under a new .comparison-matrix class so none of the bare element selectors — h2, table, thead tr, tbody tr — leak site-wide. The existing #matrix id is untouched, since it is an anchor target and is also used by the Edge matrix. The caption stays hidden and the table adds no height: the decorative column backgrounds are positioned from hardcoded row metrics, and the painted boxes still align to the columns exactly. Adds a row/column length check, which previously failed deep in the render with an opaque error.
Lifts the eleven feature rows out of the component into src/data/comparison/edge-vs-gateway.ts and takes them as an optional prop, matching how the CE-vs-PE table is parameterised. The empty td that opened the header row becomes a th scope="col" with hidden text, so the label column has a header and the row a complete set. It keeps the transparent background and zero padding the td had. Value icons become decorative beside hidden text rather than carrying an aria-label on a bare svg. Extraction of this table goes from 29 empty cells to none. Two things this skin depends on, both verified unchanged: Column widths are now emitted inline on the col elements as well as through the col classes. A fixed table whose col widths become unreachable falls back to equal quarters, which would have turned 40/20/20/20 into 25/25/25/25. The column boxes are painted positionally — td:nth-child(2..4) supply each column's colour and side borders, and tr:last-child adds the 3px base and the rounded bottom corners. nth-child counts every element child regardless of type, so promoting the first header cell from td to th does not shift the numbering. No row or cell may be inserted, reordered or appended here, and there must be no tfoot. The style block becomes is:global in the same commit as the markup move, nested under a new .edge-matrix class so the bare h2, table, thead and tbody selectors cannot leak. Both media queries move with it. The off-vocabulary values — Low, Medium to Low, High to Medium, Data Collection — are left alone: they are visible copy and already extract as text.
Starlight's Page component already wraps every one of these routes in a main, so the page-level one was a second, nested landmark. Each becomes a div keeping its class. Twenty pages plus the use-case and case-study layouts; verified by counting the rendered output, which now reports exactly one main on every affected route. Two couplings the change had to carry: All delegated pricing interaction — product tabs, sub-tabs, the FAQ scroll, the region and billing toggles — hangs off a single root looked up as "main.pricing-page". That selector loses its element and is now ".pricing-page"; the comments naming it are updated with it. Five product pages had no class on their main and styled it through a bare element selector in an is:global block, so the rule was reaching Starlight's main as well as their own. They gain a page-main class and the rule keeps main alongside it, so neither element changes width. The two remaining classless mains, on trainings and mqtt-broker, have no rule attached and become plain divs.
This table already had text in all 88 value cells, but it was the one table on the site not announced as a table at all: display:block, the GitHub-markdown overflow idiom, removes the table role in every engine. The horizontal scroll moves to a wrapper that is a labelled, focusable region, and the table goes back to display:table. The header row gains scope="col". The category and feature columns were both plain cells, so the two-level row header is now explicit: the category spanning a group is a th scope="rowgroup", and the feature is a th scope="row". Seven categories, twenty-seven features — note one category, Support, spans a single row and carries no rowspan, so it reads as a feature at a glance and is easy to mis-tag. Promoting those cells to th would have made them bold and centred, so the weight rule is now scoped to thead and the body headers are pinned back to normal weight and start alignment. Measured before and after: table 980x1085 at the same origin, and every cell type identical on weight, alignment, padding, width and position. Deliberately not routed through DataTable. This page's style block is scoped on purpose — the comment in it explains that .markdown-body needs that specificity to beat LegalLayout's global rules, and LegalLayout sets text-align: justify on the container at equal specificity but later in source order. Making the block global to satisfy the skinning contract would hand those rules the win. The table is authored here, so keeping it in place avoids the hashing problem entirely.
This section was a div grid, so nothing about it was a table to a screen reader or to text extraction. Each group card becomes its own table: the card is the table, the group title is its caption, and each row is a tr whose first cell is a th scope="row". Every table carries a zero-height header row of its own, and the visible label card on top is marked aria-hidden so those labels are announced once rather than per group. The row-wrapping anchor is gone. Its accessible name used to be the entire row including both values; the link now lives in the row header and is stretched over the row, so the name is just the feature label while the whole row stays clickable. Hover moves to tr:has(a[href]). Values are text in every cell: the mark becomes decorative with hidden "Yes" beside it, and the blank non-breaking space becomes hidden "No". 105 cells, none empty. Notes on the parts that were easy to get wrong: The white card is drawn as a 4px border split across the caption and the table rather than as padding, because a caption is a sibling of the table's grid box and the table's own padding, background and radius never reach it. The radius splits with it and the inner corners compute to zero, matching the square header corners. Column widths are plain percentages, recomputed per breakpoint. The formula the ticket specifies, calc(20px + (100% - 40px) / 3), does not work: a calc() containing a percentage is silently dropped on a col element — and on a header cell too — leaving the columns as equal thirds, which is the exact misalignment the formula was meant to avoid. The percentages fold the row's 20px side padding into the first and last columns so the row divider still spans the full card width. Padding, height and the divider all moved from the row onto the cells: a row in the separated border model ignores all three. The hover bar gets an explicit top, having previously been centred by the flex row. Verified against measurements of the previous render at 1440, 900, 500 and 375: the card, caption, divider span, feature text origin and every mark position match, and the marks land within 0.02px. One accepted deviation, on the TBMQ page only: column widths are table-wide, so the group mixing a text-link row with an icon row shifts that icon 15px at 375px.
All four were introduced by promoting a cell from td to th, or by the change of containing block that came with it. Each is measured. services/development-services: the Fast-delivery label column is styled by tbody td:first-child, which stopped matching once the labels became th. The column lost its blue background, its left alignment and its square corners, rendering identically to a value cell beneath a header that still painted blue — visible at any viewport above 960px, which is the only width where that table renders at all. tr:last-child td was orphaned the same way, leaving the last label cell with a border its neighbours drop. Both selectors now accept either cell type. Landing/ComparisonTable, hover target: tr:has(a[href]) also matches a row whose only link is a text link in a value cell, so three rows across two pages showed the full row-hover affordance — background, label growth and the left bar — while being inert. Previously those rows were divs, which [href] never matched. The selector now requires the link to be in the row header. Landing/ComparisonTable, hover bar: the bar was offset by a constant chosen for the row's resting height, but the same hover rule grows the label's line height, so every row grew and the bar sat 3px high on all 27 of them. It is centred now, which also holds when a label wraps. ce-vs-pe-diff: the new scroll wrapper is a block formatting context, so the table's own bottom margin can no longer collapse out of it and was added to the wrapper's. The gap to the following rule went from 24px to 40px. The table's margin is zeroed inside the wrapper. Also collapses a comment that had been left stacked on top of an earlier draft of itself.
Every table had a caption, which satisfied the rule, but most captions
said nothing on their own. A consumer that lifts one table out of the
page got "Additional features", "Feature comparison", "Specifications",
or — on the two product pages that render seven tables each — a bare
group name like "Platform Core", with no mention of the product or the
editions being compared. The cells were machine-readable; the table had
no subject.
Each caption now names it:
Additional features -> ThingsBoard Private Cloud: additional
features by plan
Feature Comparison Matrix -> ThingsBoard Community Edition,
Professional Edition and Cloud
Feature comparison -> Google IoT Core compared with
ThingsBoard Professional Edition
Feature Comparison -> ThingsBoard Community Edition compared
with Professional Edition
Specifications -> <partner name> hardware specifications
Fast delivery -> why each capability matters and what it
gives
<use case> overview -> high-performance compared with
traditional
The pricing and matrix components take an optional caption so the page,
which is where the product context lives, can supply it.
On the two comparison-grid pages the caption is the visible group header
bar, so its text could not change. The context is appended there as a
visually-hidden suffix instead: the bar still reads "Platform Core"
while the table's accessible name reads "Platform Core — ThingsBoard
Community Edition compared with Professional Edition". TBMQ passes its
own wording. Verified the visible text, the header bar's painted box and
the mark positions are all unchanged.
"Skin" and "skin-owned" appear nowhere else in this repo — they were invented in these comments and never defined, so a reader had to work out that they meant the calling component's own CSS. The comments now say that, and name the properties DataTable deliberately leaves alone rather than gesturing at "box metrics". Comments only; no rendered output changes.
Hovering a row grew its label's line-height from 24px to 28px, which took the row from 50px to 54px and shifted everything below it — the table moved under the pointer. The font still steps up a pixel, so the emphasis is unchanged, but the line box no longer grows. This was not introduced by the table conversion: the flex row it replaced had min-height: 50px and the same line-height growth, so it jumped by the same 4px. Measured across every row on both pages that render this component, at 1440px and 901px — the only widths where the hover rule applies: maximum height change 0px, and no label re-wraps at the larger font.
The Gantt-style timeline was the one in-scope table with no caption, no column scopes and no row headers. It now has all three: a hidden caption naming the table, scope="col" on the nine header cells, and the seven step labels promoted to th scope="row". Three rules needed widening first, or the promotion would have shown: The bare `th` rule was not scoped to thead, so a body header would have taken the 18px header font and an underline on every row. Two `td:first-child` rules carry the whole step-label column — its padding, size, colour and top alignment, plus the bold-on-row-hover — and would have stopped matching. A th also centres by default, where the td inherited start. The 49 empty week cells are unchanged. Emptiness is the data there: a step is simply not active that week, and each filled cell already states its own duration, so extraction loses nothing by their being blank.
The seven category headers carried scope="rowgroup", but the table has a single tbody, so each was asserting itself as the header for every remaining row in it — a Device Management row could resolve its group header as Platform Core. They now carry no scope at all, leaving the spec header-assignment algorithm to scope each one to exactly the rows its rowspan covers, which is what these cells mean. The feature column keeps scope="row" on all 27 rows and the header row keeps scope="col" on all four. The alternative — one tbody per category, which is what scope="rowgroup" is designed for — was rejected because the zebra striping is tr:nth-child(2n) and restarts inside each new tbody. Group four has an odd row count, so the parity flips from group five on and seven rows change shade. Verified after this change that all thirteen striped rows are exactly where they were.
Two ways this component could still produce the empty cell it exists to prevent. A string value fell through to be rendered verbatim, so an empty or whitespace-only string emitted a cell with no text and a green build. It now throws, naming the component and suggesting a real value \- an empty cell in a comparison table is an authoring mistake, not a state worth rendering. The hidden Yes/No text used a class declared only in DataTable, so the words rendered visible on any page using this component inside a hand-written table. It now declares the class itself, from the shared mixin. Both components declaring it is intentional \- each has to work alone, and the rule is identical either way. Verified on a throwaway page with no DataTable present: the spans are 1x1 and clipped, the checkmark still draws, a string value still renders as real text, and a blank value fails the build with the new message.
The id a caption carries, so its scroll region can name itself, was derived by the same unguarded slug expression in two components, and had three ways to produce an unusable value. Slugifying keeps Latin alphanumerics only, so a caption in any of the site's non-Latin locales — or one made of punctuation — reduced to nothing and emitted a bare prefix. Two such tables on a page collided. There is now a hash fallback, so an id is never bare. An override of the empty string was accepted, because ?? guards only null and undefined. A caller computing the override from data with a missing field would emit id="" and leave the region unnamed. Both components now treat a blank override as absent. SectionTable repeated the derivation and offered no override at all, so a collision there could not be worked around. It takes the same optional prop as DataTable now, and both call the shared helper. Verified against English, Chinese, Arabic, punctuation-only and a whitespace override: five distinct ids, none bare, no duplicate or empty id on the page, and every aria-labelledby resolving to its caption. The ids on the four live pages are unchanged. What this does not fix: two captions with identical text still derive the same id. A component cannot see its siblings, so that stays the caller's job — which is what the override is for, and why SectionTable needed one.
# Conflicts: # src/components/Landing/ComparisonTable.astro # src/pages/products/mqtt-broker/index.astro
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 39 changed files in Make the comparison tables semantic and leave one main per page. Left 20 comments inline.
The <main> removal itself checks out: BaseLayout renders through <StarlightPage>, which wraps the slot in Starlight's own <main id="_top">, so every page-level <main> really was a nested second landmark. The skip link targets #_top (Starlight's main), starlight-toc.ts's main [id] query still resolves against the outer main, and nothing in the tree reads main.pricing-page or main[data-slug] any more — the pricing delegation root and all four .comparison-tooltip-* → .pc-comparison-tooltip-* renames are consistent across CSS, TS and the querySelector calls. LegalLayout genuinely had no landmark and correctly gains one. What I'd push back on is not the decision but its execution: 20 pages were edited by hand into four different wrapper conventions, five pages keep a now-dead main, in the selector, one page is left with a classless <div> that does nothing, and the new "exactly one main per page" invariant has no guard — so the first page authored from an older template silently reintroduces the defect.
On "nothing changed visually": the cell-level arithmetic is genuinely careful — the 4px caption border + 20px cell padding reproduces the old 24px text origin exactly, the :is(td, th) sweeps through development-services catch every selector the td→th conversion would have orphaned, and I could not find a positional selector (tr:nth-child, :first-child) that the new <caption> displaces. Two things are not settled, though. The stretched row link depends on position: relative applying to a <tr>; if WebKit still computes that as static, the overlay re-anchors to .comparison-inner — the whole comparison section, all seven tables — and one row's link swallows clicks across the entire section. That is a hard blocker, not a cosmetic risk, and it needs the Safari check before merge. And the column widths are now 21 hand-measured four-decimal percentages with no recorded derivation, so "unchanged today" and "stays unchanged" are different claims.
On maintainability, the recurring theme is that the PR introduces a primitive and then works around it: the largest new table (seven tables in Landing/ComparisonTable) bypasses DataTable entirely and hand-rolls the caption, colgroup, zeroed thead and hidden Yes/No text; four of DataTable's ten props have no caller, including the one that would have served that very case; the new visually-hidden mixin gets four different class names plus one hand-inlined copy; and the zero-height-caption trick is written out nine times, comment and all. None of that is broken, but it means the next person adding a table has two competing patterns and no signal about which is intended.
Additional findings
These are about code outside the PR's diff — spotted while reading surrounding context.
- src/pages/blog/index.astro:330 — the comment
The inline script at the top of <main> may have added .is-entering...is now stale; that page's<main>became<div class="blog-page">in this PR. - src/pages/partners/affiliate.astro:564, src/components/LanguageSwitcher.astro:135, src/components/PromoBanner.astro:369 — three more hand-rolled visually-hidden blocks predating this PR. Now that
@mixin visually-hiddenexists they're candidates for a mechanical follow-up, which would also settle on one class name repo-wide.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| &::after { | ||
| content: ''; | ||
| position: absolute; | ||
| inset: 0; |
There was a problem hiding this comment.
The PR description calls the Safari check on this "worth one manual check before merge" — I'd frame it harder, because the failure isn't a small misalignment. The nearest positioned ancestor above <tr> is .comparison-inner (line 269, position: relative), which wraps all seven group tables. So if WebKit computes position on a table row as static, every row's ::after stretches over the entire feature-comparison section, not over its own row — and the last one in paint order takes clicks for the whole block. A visitor clicking "Rule Engine" would land on whatever the final linked row points at.
That argues for the deterministic option even at the cost of hit area: move the containing block to th.col-feature { position: relative }. Cells have been reliably positionable in every engine for years, and the worst case then is a smaller click target rather than a section-wide misdirect. If you'd rather keep the row-wide target, this needs an actual Safari run on /products/thingsboard-pe/ recorded in the PR before merge, not an open question.
| <td class="col-ce"> | ||
| {row.ceText ? ( | ||
| row.ceLinkHref ? ( | ||
| <a href={row.ceLinkHref} target="_blank" rel="noopener noreferrer" class="cell-link">{row.ceText}</a> |
There was a problem hiding this comment.
The full-row overlay and these cell links can't coexist. a::after { inset: 0 } in the row header is positioned, .cell-link is not, so the overlay paints above it and eats the click — any row that has both href and ceLinkHref/peLinkHref loses its cell links silently.
Today nothing hits it: Support Model is the only row with cell links and it has no href. But ComparisonRow advertises both as independent optionals, so this is a trap someone will fall into by adding one field to a data row, with no error to explain it. Either give .cell-link a position: relative; z-index: 1 so it stays above the overlay, or reject the combination at build time the way DataTableValue rejects a blank value — you clearly prefer the loud-failure approach elsewhere in this PR.
| <div class="legal-page"> | ||
| {/* <main>, not a div: these pages do not route through Starlight, so nothing | ||
| else supplies the landmark. */} | ||
| <main class="legal-page"> |
There was a problem hiding this comment.
This is where I'd most like to see something added rather than changed. The PR establishes two invariants that are invisible when violated — exactly one <main> per rendered page, and no icon-only table cells — and neither gets a guard, even though the repo already has the pattern for exactly this: scripts/lint-dual-render.ts, whose own header makes the same argument (builds clean, typechecks clean, only renders wrong).
A ~30-line scripts/lint-landmarks.ts counting <main occurrences per dist/**/*.html would pin the 22-file change down permanently. Without it, the invariant is recorded only in this diff, and the next page copied from an older template reintroduces the nested landmark with nothing to catch it — which is the same way the 20 pages got there in the first place. That's the difference between fixing this once and fixing it again next quarter.
|
|
||
| main { | ||
| main, | ||
| .page-main { |
There was a problem hiding this comment.
Five product pages keep a bare main in this selector for an element they no longer render. In mobile/ and mobile-pe/ the block is scoped, so main can never match and it's simply dead. Here, in paas/ and in thingsboard-edge/ the block is is:global, so width: 100% now applies only to Starlight's <main> — that was already true before the change so it isn't a regression, but it's precisely the accidental cross-boundary rule this PR is otherwise cleaning up. Dropping the main, half would make the point of the change legible.
The wrapper naming is also inconsistent across the 20 pages: five get a newly-invented .page-main (an odd name for the element that deliberately isn't main), development-services keeps .page-wrap, most keep their existing page class, and services/trainings ends up with a classless <div>. If the wrapper is just a styling hook now, something like .page-body applied uniformly would read better than three conventions.
| description="Practical ThingsBoard training led by experts — 15 two-hour sessions for teams up to 8 engineers, covering visualization, admin, SCADA, and more." | ||
| > | ||
| <main> | ||
| <div> |
There was a problem hiding this comment.
This one can go entirely — it's a classless <div> with no styles anywhere in the file and no siblings, so it wraps a slot's children for no reason. <main> → <div> was the mechanical edit, but here the element only existed to be the landmark.
| // Widths are emitted inline on <col>, which `table-layout: fixed` honours in every | ||
| // browser; the per-class width rules below are a redundant fallback. | ||
| const columns: DataTableColumn[] = [ | ||
| { label: '', srLabel: 'Feature', width: '30%', thClass: 'pc-comparison-feature-col' }, |
There was a problem hiding this comment.
All three DataTable callers open their column array with this same incantation — an empty label meaning "no visible header", plus an srLabel that all three set to the literal 'Feature', which is also DataTable's own ?? 'Feature' fallback. That's one convention encoded in four places.
A dedicated rowHeaderLabel?: string (defaulting to 'Feature'), with columns holding only the value columns, would make the shape self-evident and drop the empty-string sentinel — and it would make the two column arrays in ComparisonMatrix (columns from props, dtColumns derived with the feature column prepended) collapse into one, which is currently the only reason that component has to maintain both.
| * here — a component cannot see its siblings — so a caller rendering two tables with | ||
| * identical captions on one page must pass an explicit id for at least one of them. | ||
| */ | ||
| export function captionId(text: string, prefix: string): string { |
There was a problem hiding this comment.
There's a near-identical routine already in the tree: src/components/Pagination/PerPageSelector.astro:19 slugifies with the same .toLowerCase().replace(/[^a-z0-9]+/g, '-') pair to build a deterministic aria-labelledby target. Same purpose, same problem, slightly different edge handling — worth one slugId(text, prefix) util rather than a caption-specific second copy?
Two smaller notes: prefix is a magic string picked per call site ('dt-caption', 'ih-table') with nothing relating them, and the non-Latin fallback hash has no reachable caller — the marketing and landing pages that render these tables aren't localised, so that branch is speculative.
| columns: string[]; | ||
| rows: string[][]; | ||
| /** Overrides the derived id. Needed only when one page has two identical headings. */ | ||
| captionId?: string; |
There was a problem hiding this comment.
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.
| <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"> |
There was a problem hiding this comment.
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.
| // The wrapper is a block formatting context, so the table's own bottom | ||
| // margin can no longer collapse out of it and would be added twice. | ||
| table { | ||
| margin-bottom: 0; |
There was a problem hiding this comment.
Is this rule doing anything? The margin-bottom: 16px it's guarding against was moved off table and onto .table-scroll in this same diff, so unless a margin arrives from outside this page's <style> block, the table has no bottom margin to zero — and the comment describes a double-margin that can't occur. Worth deleting both, or keeping the rule with a comment naming where the margin actually comes from.
Also on line 65: the new wrapper's contents weren't re-indented, so the ~120-line <table> and the closing </div> sit at the same depth as the opening <div> and read as siblings. Suggests pnpm format didn't run over this file.
2 participants
Makes the comparison tables on the marketing pages readable as tables — by a screen reader and by anything that reads the HTML as text — and leaves exactly one
mainlandmark per page. No visual change.Ref: PROD-8679
The problem
Values were carried by icons alone, so a text extraction of a row returned nothing:
What changed
A shared primitive.
src/components/DataTable.astroemits caption → colgroup → thead (th scope="col") → tbody slot; callers pass rows whose first cell is ath scope="row". It sets no box metrics at all — the callers disagree on every one of them, so appearance stays with the caller.DataTableValue.astrorenders a cell's value as text in every case, with the icon marked decorative.Eight tables migrated or repaired. Pricing, the Cloud and Edge matrices, the Google IoT Core comparison, the CE-vs-PE reference table, the hardware spec table, the use-case comparison, and both tables on the development-services page.
The feature comparison grid became real tables. It was a div grid, so nothing about it was a table to begin with. Each group card is now its own table with its title as the caption. The row-wrapping anchor is gone — its accessible name was the entire row including both values; the link now lives in the row header and is stretched over the row, so the whole row stays clickable.
One
mainper page. Starlight already emits one, so the page-level element was a second, nested landmark. 20 pages plus two layouts.LegalLayouthad the opposite defect — no landmark at all on its pages — and gained one.Results
/pricing/empty cells/products/paas//products/thingsboard-edge//products/thingsboard-pe//ce-vs-pe-diff/display: blockstripped the table rolemainStray end tag "td")Every caption also names its subject, so a table lifted out of the page still says what it compares —
Platform CorebecamePlatform Core — ThingsBoard Community Edition compared with Professional Edition.Verification
Run against the merged branch, after main was brought in.
pnpm build:fastclean (4,349 pages) ·lint:linkcheck·lint:dualrender·lint:slugcheck·lint:steps·eslint·astro checkall pass. W3C Nu on the 20 affected pages: 0 table-structure errors. The 227 remaining Nu errors are pre-existing and untouched —<source>withoutsrc,<button>inside<a>, and the cookie banner'saria-label, which the ticket lists as out of scope.Geometry was measured before and after at 1440 / 900 / 500 / 375 px on the pages that changed most: card boxes, caption painted regions, divider spans, text origins and every icon position match, with icons within 0.02px.
Merge notes
Main moved 10 commits while this was in progress, two of which overlapped:
products/mqtt-broker/index.astrowas deleted (TBMQ related info migration to new website #585, refactor(tbmq): drop TBMQ legal pages superseded by tbmq.io #630). Its edits here are dropped, andLanding/ComparisonTablenow has a single consumer. This also removes the one visual deviation this PR previously carried: TBMQ had a group mixing a text-link row with an icon row, and since column widths are table-wide, that icon shifted 15px at 375px. With the page gone, there is no deviation left.externalLinkAttrs()(TBMQ related info migration to new website #585) on the same lines this PR replaced. Adopted for the row link, where the inline test and the helper agree.One thing deliberately not folded in: the two cell-links still set
target/relexplicitly. The CE one is external, but the PE one is a local path, and both currently open in a new tab — so switching them to the helper would change behaviour, since its policy gives local paths neither attribute. That looks like a pre-existing policy violation worth its own change rather than a silent rider here.Two things reviewers should know
Not verified: Safari. The stretched row link anchors its overlay to
tr { position: relative }. That is spec-correct, but WebKit has historically computedpositionon a table row as static, which would re-anchor the overlay to an ancestor. Worth one manual check on/products/thingsboard-pe/before merge; the deterministic alternative is to move the containing block onto the cell, at the cost of shrinking the hit area from the row to the label column.The formula in the ticket does not work.
calc(20px + (100% - 40px) / 3)is silently dropped on acolelement — and on a header cell — leaving the columns as equal thirds, which is the exact misalignment it was written to prevent. Column widths are plain per-breakpoint percentages instead, derived from the measured render.Left out, deliberately
Medium to LowandSQL + NoSQLis untouched — normalising it is a content decision./docs/*, outside this ticket's scope. They share the same missing-scope defect and are a mechanical follow-up now that the primitive exists.