refactor(pagination): one shared Pagination component for all six surfaces#477
refactor(pagination): one shared Pagination component for all six surfaces#477rusikv wants to merge 4 commits into
Conversation
Six surfaces each carried their own pagination: IoT Hub's link-based PaginationLink + runtime updater, a button shell shared by Device Library and Partners (with the windowing JS copy-pasted into both), and bespoke innerHTML renderers on the blog and case-studies indexes. Three drifted copies of the windowing algorithm, four styling vocabularies, and recurring a11y gaps. Replace all of it with src/components/Pagination/: - Pagination.astro — one nav, two modes: basePath renders real anchors (SSG, rel=prev/next, crawlable); without it, buttons that dispatch bubbling tb-pagination:page-change events. Numbered list on desktop, compact 'Page X of Y' row below lg. Brand tokens (indigo/lavender current-page pill), aria-current in both modes, focus-visible rings. - pagination-client.ts — updatePagination() rebuilds the lists for client-driven surfaces; restores keyboard focus to the equivalent control after each rebuild (fixes a focus-loss bug all the old implementations shared). - pagination-shared.ts — the single windowing algorithm + strings. - PerPageSelector.astro — the IoT-Hub-only items-per-page dropdown, composed in via the 'controls' slot from consumer files so its script/styles ship only to IoT Hub pages (Astro bundles per import graph, not per render). Consumers keep what is theirs: URL state (?page= on blog), scroll behavior, sessionStorage restore (case-studies), and the dynamic search pipeline (IoT Hub) — the component only renders and reports. Intended deltas: unified look on all surfaces, blog gains prev/next chevrons, grid surfaces show full number rows up to 7 pages (was 5), Device Library/Partners drop the redundant always-visible 'Page X of Y' line (lives in the sub-lg compact layout now).
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 16 changed files in refactor(pagination): one shared Pagination component for all six surfaces. Left 6 comment(s) inline.
The refactor is clean and well-documented: the link/interactive mode split is coherent, buildPages/chevron geometry is centralized in pagination-shared.ts so static and runtime renders can't drift, totalPages is clamped to >= 1, the deep-link ?page= path and the double-init guards check out, and repo-wide greps confirm no dangling references to the deleted components, classes, or old event names. The comments below are mostly design/maintainability observations rather than correctness defects — one minor focus-management nuance, the rest about coupling and API discoverability.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| ...nav.querySelectorAll<HTMLElement>(`[data-nav-role="${role}"]`), | ||
| nav.querySelector<HTMLElement>('.tb-pagination__page.is-current'), | ||
| ].filter((el): el is HTMLElement => !!el); | ||
| candidates.find(visible)?.focus(); |
There was a problem hiding this comment.
When prev/next is clicked to the first/last page, that chevron is rebuilt as disabled (is-disabled + aria-disabled) but is still a focusable, still-visible <button>, so candidates.find(visible) parks focus right back on it — a control the user can no longer activate. Since the candidate list already falls through to .tb-pagination__page.is-current, would it be worth skipping disabled candidates here (e.g. filtering out .is-disabled) so boundary navigation lands focus on the current-page button instead of a dead chevron?
| class: className = '', | ||
| } = Astro.props; | ||
|
|
||
| const isLinkMode = typeof basePath === 'string'; |
There was a problem hiding this comment.
Link vs. interactive mode is selected implicitly by whether basePath happens to be passed. That's a meaningful behavioral fork — real anchors + SSR navigation vs. event-emitting buttons rebuilt by JS — hinging on one optional prop's presence, and it's easy to get subtly wrong from a call site: a consumer that means to be interactive but passes a basePath silently gets anchors and no events. Would an explicit mode: 'link' | 'interactive' prop (with basePath required only for link mode) make the contract more discoverable? The doc comment is good; it's just that the type signature itself doesn't express the coupling.
| <ul class="tb-pagination__pages tb-pagination__pages--numbers"> | ||
| <li> | ||
| { | ||
| isLinkMode && hasPrev ? ( |
There was a problem hiding this comment.
This prev/next chevron branch (link <a> / interactive <button> / disabled <span>) is written out four times in the template — prev and next, each duplicated across the --numbers list and the --compact row — differing only in direction, rel, label, and data-nav-role. A small local helper (a function returning the fragment, or an inline sub-component parameterised by direction/target/enabled) would collapse the four near-identical copies into one and keep the variants from drifting. Not load-bearing, but it's the densest part of the file to read.
| } | ||
|
|
||
| // Same geometry as Chevron.astro (24px viewBox, 2px round stroke). | ||
| export const CHEVRON_PATHS = { |
There was a problem hiding this comment.
These two path strings duplicate paths.left/paths.right in Chevron.astro. The comment honestly flags the coupling, but a comment won't keep them in sync — if the icon is ever redrawn, the runtime-built chevrons here drift from the build-time <Chevron> ones that Pagination.astro renders. Since Chevron.astro is essentially a thin wrapper over a path map, could that map be hoisted into a shared TS module that both Chevron.astro and this file import, rather than re-declaring the d-strings?
| perPageRoot.querySelectorAll<HTMLButtonElement>('[data-per-page-option]').forEach((opt) => { | ||
| const isSelected = opt === target; | ||
| opt.classList.toggle('iot-hub-pagination__per-page-option--selected', isSelected); | ||
| opt.classList.toggle('tb-pagination__per-page-option--selected', isSelected); |
There was a problem hiding this comment.
applyPageSizeToUi reaches into PerPageSelector's internal DOM contract — [data-per-page-option], [data-per-page-value], [data-per-page-label], and this tb-pagination__per-page-option--selected class — to reflect a restored ?pageSize= back into the dropdown. PerPageSelector already owns that markup and does the identical class/aria/label toggling in its own option-click handler, so the selection logic now lives in two places and a rename on either side silently breaks URL-restore here with no type safety to catch it. Could PerPageSelector expose a small imperative helper (e.g. setPerPageValue(root, n)) or react to a tb-pagination:set-page-size event so consumers don't hand-poke its internals? (It does mirror the existing applySortToUi/IotHubSort pattern, so it's at least locally consistent.)
| renderResults(items); | ||
| if (paginationNav) { | ||
| updatePaginationDynamic(paginationNav, { currentPage, totalPages }); | ||
| updatePagination(paginationNav, { currentPage, totalPages }); |
There was a problem hiding this comment.
Five of the six interactive consumers call updatePagination with hideOnSinglePage: true; this IoT Hub path omits it and instead leans on the SSR page.lastPage > 1 guard plus showFetchError toggling nav.hidden by hand. The effect differs: if a runtime filter narrows results to a single page here, the nav stays visible showing just "1" rather than hiding like the other surfaces. Intentional asymmetry, or just an omission? Either way, a one-line comment at this call site noting the deliberate divergence from the shared default would save the next person from "fixing" it to match.
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 16 changed files in refactor(pagination): one shared Pagination component for all six surfaces. Left 6 comments inline.
This is a clean, well-executed dedup refactor. Repo-wide greps confirm no dangling references to the removed components, events (iot-hub-page:change/iot-hub-page-size:change), classes, or strings; the blog's totalPages is correctly recomputed in render(); and the two consumer-side changes (the case-studies double-init guard and the focus-restore on rebuild) are genuine improvements over the old code. I didn't find functional bugs — the inline comments are all maintainability / consistency / API-shape observations on the new shared component.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| } | ||
|
|
||
| // Same geometry as Chevron.astro (24px viewBox, 2px round stroke). | ||
| export const CHEVRON_PATHS = { |
There was a problem hiding this comment.
These two path strings are a verbatim copy of the left/right entries in Chevron.astro's path map, and the comment right above ("Same geometry as Chevron.astro") spells out exactly the copy-paste invariant this PR is otherwise trying to eliminate. The SSR path already renders <Chevron> directly — only the runtime rebuild in pagination-client.ts needs the raw strings. Could the geometry come from a single source (e.g. Chevron.astro importing these paths, or both reading one shared constant), so a future tweak to the icon can't silently drift between the static and dynamic renders?
| right: 'm9 6 6 6-6 6', | ||
| } as const; | ||
|
|
||
| export function formatPageSummary(current: number, total: number): string { |
There was a problem hiding this comment.
Every other user-facing string here is either centralised in PAGINATION_STRINGS or overridable via props (ariaLabel, prevLabel, nextLabel), but the compact "Page X of Y" summary is hardcoded English with no override hook. On a 14-language site this becomes the one pagination label that silently stays English in the sub-lg layout. Worth routing it through PAGINATION_STRINGS as a template (or accepting it as a prop) for consistency with the rest of the component.
| class: className = '', | ||
| } = Astro.props; | ||
|
|
||
| const isLinkMode = typeof basePath === 'string'; |
There was a problem hiding this comment.
The link-vs-interactive mode is selected implicitly by whether basePath is set, and several props only make sense in one mode — hidden is interactive-only per its own doc comment, and currentPage/totalPages are effectively meaningless in interactive mode since the client rebuilds them at runtime. Nothing in the Props type signals which props pair with which mode, so it's easy to pass a combination that does nothing. Grouping the props by mode in the doc comments — or a discriminated mode prop — would make the two-mode design harder to misuse.
| <ul class="tb-pagination__pages tb-pagination__pages--numbers"> | ||
| <li> | ||
| { | ||
| isLinkMode && hasPrev ? ( |
There was a problem hiding this comment.
The prev/next chevron is authored four times in this template: a 3-way (link / interactive button / disabled span) branch each for prev and next in the numbers list, plus two more 2-way branches in the compact row. They differ only in direction, target page, and label, so it's easy for the variants to drift — e.g. someone fixing an a11y attribute on one and missing the other three. A small local renderChevron(direction, enabled, hrefOrRole, label) fragment would collapse most of this duplication.
|
|
||
| function restoreFocus(nav: HTMLElement, role: string | null): void { | ||
| if (!role) return; | ||
| const visible = (el: HTMLElement) => el.offsetParent !== null; |
There was a problem hiding this comment.
Two notes on restoreFocus:
offsetParent !== nullis a leaky visibility probe — it works here only because the numbers/compact rows are toggled withdisplay: nonevia media queries. If a future layout variant ever hides a row withvisibility/opacity/clipinstead, the desktop-vs-mobile focus pick would break silently. Worth a one-line comment pinning down thedisplay:none-only assumption.- When the user pages to the last (or first) page via a chevron, that chevron has just become
is-disabled— but it's still focusable (nodisabledattribute, onlyaria-disabled+pointer-events: none), so focus is restored onto a dimmed, non-interactive control. Sincedata-nav-rolematches before the.is-currentfallback, that boundary case lands focus on the disabled chevron rather than the current page. Preferring the current-page button when the captured role resolves to a disabled control might feel less surprising.
| perPageRoot.querySelectorAll<HTMLButtonElement>('[data-per-page-option]').forEach((opt) => { | ||
| const isSelected = opt === target; | ||
| opt.classList.toggle('iot-hub-pagination__per-page-option--selected', isSelected); | ||
| opt.classList.toggle('tb-pagination__per-page-option--selected', isSelected); |
There was a problem hiding this comment.
applyPageSizeToUi re-implements PerPageSelector's own selection logic — the tb-pagination__per-page-option--selected class toggle, aria-pressed, label text, and the data-per-page dataset are all also owned by setupPerPage inside the component. So two files now have to agree on PerPageSelector's private class names and DOM contract, which cuts against the otherwise-clean black-box boundary (events + data attributes) the shared component establishes. Exposing a small helper like setPerPageValue(root, n) from the component would keep that markup owned in exactly one place.
- Decouple PerPageSelector from the nav: new PaginationBar wraps the nav and the selector as siblings, so hiding the nav on a single page (or fetch error) no longer strips the items-per-page control — closes the trap where raising page size to one page removed the only way to lower it again. (review thingsboard#5) - Enable hideOnSinglePage on the IoT Hub runtime path now that the selector survives the nav hiding; route error-hide to the bar. (thingsboard#6) - Extract per-page-client.ts (setPerPageValue/setupPerPage) so URL-restore stops re-implementing the selector's internal toggling. (thingsboard#5) - restoreFocus skips .is-disabled controls so boundary navigation lands on the current page, not a dead chevron. (thingsboard#1) - Collapse the four duplicated prev/next chevron branches into PaginationChevron.astro. (thingsboard#3) - Single chevron path map in chevron-paths.ts, shared by Chevron.astro and pagination-shared.ts. (thingsboard#4) - Guard pageUrl so it only builds a URL in link mode.
Resolves conflicts from develop's Device Library removal (thingsboard#491) vs the shared-Pagination refactor: - DeviceLibrary/* removed (accept develop): the feature is gone, so the refactor's edits to DeviceLibrary.astro are moot. - Partners/Pagination.astro deleted: Partners now uses the shared Pagination component, not the old per-surface one develop moved here. - Partners/PartnerLibrary.astro: keep the shared-Pagination migration; point SearchBar at the moved ./SearchBar.astro.
- Replace the implicit basePath-presence mode inference with an explicit discriminated-union `mode: 'link' | 'interactive'` prop. Illegal combos (basePath in interactive mode, hidden in link mode, missing mode) are now astro-check errors instead of silent runtime surprises. The runtime markup contract and updatePagination are untouched, so IoT Hub's SSR-link -> runtime-interactive hybrid is unaffected. (review thingsboard#2) - Centralize the compact 'Page X of Y' summary into PAGINATION_STRINGS as a {current}/{total} template so every user-facing string lives in one place. - Document restoreFocus's display:none-only assumption behind the offsetParent visibility probe.
2 participants
What
Extracts pagination into a single shared component —
src/components/Pagination/— and migrates every surface that paginates to it:PaginationLink+PaginationChevron+iot-hub-pagination-update.tsinnerHTMLrenderer,cs-*classesinnerHTMLrenderer (no prev/next)Net: −389 lines, one windowing algorithm instead of three drifted copies, one styling vocabulary instead of four.
Design
basePath, page numbers are real anchors (/iot-hub/widgets/3/,rel=prev/next, crawlable, zero JS). Without it, buttons dispatch bubblingtb-pagination:page-changeevents andupdatePagination()rebuilds the nav. IoT Hub uses both in sequence: static links until search/filters activate the dynamic pipeline.?page=), scroll handling, sessionStorage restore (case-studies), API refetching (IoT Hub) all stay in consumer code — the component only renders and reports. This boundary is documented in a design-contract comment at the top of the component.controlsslot (PerPageSelector.astro, the items-per-page dropdown). Slotting instead of a prop keeps the dropdown's script/styles out of the other surfaces' bundles — Astro bundles per import graph, not per render.Improvements over the old implementations
<body>; the old case-studies code could even double-fire prev/next).aria-current="page"everywhere (previously IoT Hub only); icon buttons have aria-labels at all widths;prefers-reduced-motionrespected.--color-brand/-contrast) replace hardcoded#b3c7ff/#17181c/--sl-*colors → correct dark theme on all surfaces.lgbreakpoint on every surface.Intended visual/behavior deltas (not regressions)
lglayout shows it).Verification
astro check(0/0/0), ESLint clean on all touched files./blog/?page=2exercises the client updater + URL restore end-to-end.lint:linkchecknot yet run (no routes/links changed by this PR — pagination URLs are generated by the samepaginate()/getStaticPathscode as before).