Carousel unification: slot-only shared component, use-case dashboards, a11y fixes + site-wide astro:page-load cleanup#484
Conversation
The old Jekyll site showed an image carousel on 13 use-case pages; only 8 were migrated. Restore the missing 5 — smart-metering, water-metering, smart-retail, air-quality-monitoring, and scada-energy-management — by copying the original webp screenshots and adding the overview carousel block to each data file. Alt/title texts, image order, and dimensions match the legacy carousel data verbatim, except scada-energy-management slide 3, whose declared 1286x660 box was 1.59% off the real asset's aspect ratio (2604x1358) and is corrected to the aspect-exact 1302x679. The smart-retail and air-quality screenshots were exported near-lossless on the legacy site; they are re-encoded at webp quality 80 (matching their siblings), cutting those 10 files from 2.4 MB to 1.0 MB. The remaining 13 were already optimally encoded.
Replace the legacy owl-carousel-era chrome (thin hand-drawn SVG chevrons, 72x4 dash pagination) on the shared Landing carousel: - Arrows: 44px circular buttons with astro-icon tabler chevrons; lavender-tinted disc at rest, solid accent with contrast icon on hover, focus-visible ring. - Dots: 8px round dots; the active one morphs into a 24px pill. Uniform 28x24px touch targets replace the mobile-only override. - Colors ride on --carousel-accent: brand indigo in light theme, lavender in dark, via var(--color-brand-indigo, var(--color-product-cloud)) so it picks up the brand token once the brand unification lands while rendering identically today. - New .carousel-stage wrapper anchors the arrow overlay to the slide area (excluding dots, and the caption in the simple variant) so buttons center on the image instead of sitting low. - Home-page usecase variant: side padding 50px -> 64px to clear the round buttons.
The site serves referrer-policy: same-origin (Cloudflare managed transform), so browsers send no Referer to YouTube's embed player, which now requires one — raw iframes fail with player error 153. Swap the two raw <iframe> embeds for the YouTubeVideo component, whose referrerpolicy="strict-origin-when-cross-origin" attribute overrides the document policy (plus nocookie domain, lazy loading, and an accessible title). These were the only raw YT embeds left.
…t reduced motion The custom setInterval autoplay could start a second timer without clearing the first (drag a slide, then mouse out: pointerUp and mouseleave each call startAutoplay), leaking an interval and advancing slides at double speed. It also resumed autoplay on pointerUp even while the cursor was still over the carousel, contradicting the hover-pause behavior. Swap it for the embla-carousel-autoplay plugin already used by AdvantagesCarousel (delay from the existing autoplayInterval prop, stopOnInteraction: false, stopOnMouseEnter: true), and skip autoplay entirely when the user has prefers-reduced-motion set (WCAG 2.2.2).
Add the APG carousel pattern markup to the shared Landing carousel: role=region with aria-roledescription=carousel and an aria-label (new ariaLabel prop, set at both call sites), role=group slides labelled 'N of M', and aria-current tracking on the active dot. Also drop the Math.random() viewport id: the script finds the viewport with a class selector inside its own wrapper, so the id only made build output non-reproducible.
Move Landing/Carousel.astro to src/components/Carousel/ and generalize it
so other embla carousels can ride it:
- carousel-init.ts: client logic extracted from the inline script — lazy
embla import, IntersectionObserver init, reduced-motion autoplay gate.
Adds a carousel:select CustomEvent for master-detail consumers and
JS-built dots (from scrollSnapList) for slot mode. Initial dot/button
state is applied via direct calls since embla defers 'init' to a macrotask.
- Slot mode + CarouselSlide.astro for arbitrary slide markup; slides are
sized through --carousel-slide-basis/--carousel-slide-gutter custom
props because slot content is outside the component's style scope.
- perView={3} ladder (3/2/1 across lg/md) using loop-seam-safe
margin-right gutters instead of flex gap.
- arrowPlacement='header' renders the nav buttons in a title row above
the viewport instead of the floating overlay.
- CarouselItem href/title become optional (drops the '#' sentinel);
homeCarousel.ts imports the interface instead of re-declaring it.
- ariaLabel is now a required prop.
Delete the UseCaseCarousel wrapper — use-cases/[slug].astro calls
Carousel directly with the same mapping (title || alt caption fallback
and section margins preserved).
Zero behavior change for the homepage and use-case page carousels.
AdvantagesCarousel keeps only the card markup (in CarouselSlide slots)
and delegates all carousel behavior to Carousel with
arrowPlacement='header', perView={3}, autoplay.
Intentional changes vs the standalone implementation:
- autoplay is now suppressed under prefers-reduced-motion
- nav arrows adopt the shared indigo .carousel-button chrome
- gains role=region/slide ARIA semantics and lazy IO init
- drops the Math.random() viewport ids and the duplicated embla script
Replace the blog's hand-rolled embla wiring with the shared Carousel component in slot mode. The editorial card markup and the bordered pill-dots chrome stay page-local; only the engine moves. - Add a dotCount prop to Carousel so slot-mode dots render at SSR (no layout shift waiting for JS) instead of being built after init. - Card slides keep their full-viewport flush layout: gap is zeroed on the container directly, since the wrapper self-declares --carousel-gap and a parent-level custom-prop override would be shadowed. - Slide 0 keeps loading=eager/fetchpriority=high for LCP. - Drop the page's static embla import + initCarousel; the shared component lazy-loads embla via IntersectionObserver. Intended behavior changes vs the old implementation: - autoplay no longer dies permanently on first interaction (stopOnInteraction false + stopOnMouseEnter) and is suppressed under prefers-reduced-motion - card text is no longer selectable during drag (user-select: none), matching every other carousel on the site
On phones the featured card was inset by the blog container gutter on both sides, which looked cramped. Break the carousel section out of the container's mobile padding (var(--spacing-4); the container also drops its border below 1024px) so the card spans edge-to-edge, and drop the card's rounded corners + side borders so the flush edge reads as intentional. Negative margins rather than 100vw/transform, to avoid clashing with the transform-based section entrance animation.
The 'default' items-variant had no consumers (homepage uses 'usecase', use-cases use 'simple', blog/advantages use slot mode) and rendered markup identical to 'simple'. Collapse it: remove the variant value and its dead render branch + CSS, and stop stamping a meaningless carousel-<variant> class on slot-mode wrappers (they get carousel-slotted). Also clarify the dotCount doc comment.
The three CSS marquees (logo strip, company hero photos, visualization widgets strip) each duplicate their content for a seamless loop but animated unconditionally and announced the duplicate copy to screen readers. For each: add a prefers-reduced-motion guard that holds the strip still, and aria-hidden the duplicated half (the visualization duplicate also gets alt=""). HeroPhotoCarousel's drag handler no longer re-arms the auto-scroll under reduced motion.
Review follow-ups: - Autoplay used stopOnInteraction:false + stopOnMouseEnter:true, but mouseenter never fires on touch, so a swipe didn't pause autoplay and it kept yanking the slide away. Gate stopOnInteraction on (pointer: coarse): touch stops on swipe, mouse keeps hover-pause + resume-after-drag. - Move the data-carousel-init check inside the IntersectionObserver callback so a second observeCarousels() run (e.g. astro:page-load + DOMContentLoaded) can't double-init the same wrapper.
CarouselSlide now takes index/total and renders the "N of M" aria-label server-side, matching items-mode slides. carousel-init.ts keeps setting it as a JS fallback only when the props are omitted. Blog featured and advantages carousels pass index/total so their slides are named without relying on JS.
The simple-variant carousel renders title as a visible caption, so the
placeholder alt text ("water metering 1"…) was showing on-screen. Give
each water-metering and SCADA energy-management dashboard image a real
title (and descriptive alt for the water-metering placeholders),
written from the actual dashboard contents.
vvlladd28
left a comment
vvlladd28
left a comment
There was a problem hiding this comment.
Review summary
Reviewed 43 changed files in Carousel unification: shared component + use-case dashboards, blog & marquee a11y fixes. Left 6 comment(s) inline.
This is a clean, well-documented refactor. On the correctness side I found no hard bugs: image paths resolve through SmartImage's /src/assets/images/** glob, there are no orphaned references to the deleted Landing/Carousel.astro or UseCaseCarousel.astro, every Carousel consumer passes the required ariaLabel, the double-init race is explicitly guarded, and the marquee aria-hidden correctly targets only the duplicated copies. The inline comments are all quality/design observations — mostly about the shape of the new component's public API.
This review was auto-generated. Findings may contain errors — please verify before applying changes.
| autoplayInterval?: number; | ||
| loop?: boolean; | ||
| /** 3 shows three slides per view on desktop, two below lg, one below md. */ | ||
| perView?: 1 | 3; |
There was a problem hiding this comment.
perView as a 1 | 3 union reads awkwardly. The name suggests "slides shown per view", but 3 actually selects a fixed 3/2/1 responsive ladder and 2 is impossible — so the number doesn't map to a single observable behavior, and a future layout means another magic literal. A named mode like layout: 'single' | 'multi' would communicate intent better and be harder to misuse.
| /** Prop-driven slides. Omit and fill the default slot with `<CarouselSlide>`s instead for arbitrary markup. */ | ||
| items?: CarouselItem[]; | ||
| /** Items-mode slide design (ignored in slot mode). The usecase variant expects `href` on every item. */ | ||
| variant?: 'usecase' | 'simple'; |
There was a problem hiding this comment.
This Props surface folds two mutually-exclusive modes (items-mode vs slot-mode) into one flat interface: variant is ignored in slot mode, dotCount is ignored in items mode, arrowPlacement='header' only matters with a title, and title only matters under header placement. The JSDoc helps, but it's easy to pass a prop that silently does nothing. Now that every consumer except the home page uses slot mode, it's worth asking whether items-mode is still earning its keep — turning the home usecase slide into its own small slide component and going slot-only would shrink this API considerably.
| </div> | ||
| ) : ( | ||
| <div class="carousel-item-simple"> | ||
| {item.href ? ( |
There was a problem hiding this comment.
The simple-variant SmartImage is duplicated verbatim across the href / no-href branches (same five props). Rendering the image once and conditionally wrapping just the anchor around it would avoid keeping two identical copies in sync.
| <script> | ||
| import { observeCarousels } from './carousel-init'; | ||
|
|
||
| document.addEventListener('DOMContentLoaded', observeCarousels); |
There was a problem hiding this comment.
Registering observeCarousels on both DOMContentLoaded and astro:page-load means a hard load fires both, and each call builds a fresh IntersectionObserver over the not-yet-initialized wrappers — so you can end up with two live observers watching the same set (carousel-init.ts's flag still prevents double init, as its comment notes, but not the redundant observer). Since the site uses the view-transitions router, astro:page-load fires on the initial load too, so dropping the DOMContentLoaded listener would remove the duplication.
| <UseCaseCarousel images={data.overview.carouselImages} /> | ||
| <section class="uc-carousel-section"> | ||
| <Carousel | ||
| items={data.overview.carouselImages.map((img) => ({ ...img, title: img.title || img.alt }))} |
There was a problem hiding this comment.
Falling back to img.alt for the caption means that whenever a carousel image omits title, its long screen-reader alt-text (e.g. "ThingsBoard dashboard showing energy and water consumption, district map, and critical alarm for District B…") gets rendered as a visible caption under the slide. Alt text and a human-facing caption have different audiences and lengths. Since all the use-case images in this PR do supply a title, consider only setting title when one is authored (and letting the simple variant render no caption otherwise) rather than surfacing alt prose.
| draft: false | ||
| --- | ||
|
|
||
| import YouTubeVideo from '~/components/YouTubeVideo.astro'; |
There was a problem hiding this comment.
This new import uses the legacy ~/ alias, but CLAUDE.md asks for the @components/* alias on new code — @components/YouTubeVideo.astro here (same import was added to the calculated-fields post). Minor, but these lines are net-new in this PR so it's a cheap spot to follow the convention.
…fallback Review follow-up (thingsboard#484): the SCADA oil-&-gas drilling carousel images had no title, so the simple variant fell back to rendering their screen-reader alt text as a visible caption. Give the three images real captions (written from the dashboards), and pass carouselImages straight through instead of `title: img.title || img.alt` — CarouselImage already satisfies CarouselItem, so a title-less image now shows no caption rather than leaking alt prose.
`perView: 1 | 3` read awkwardly (3 selected a 3/2/1 responsive ladder, 2 was impossible). `layout: 'single' | 'multi'` maps to one observable behavior each. Pure rename — no behavior change.
Strip all slide-content knowledge out of Carousel.astro so it is a pure embla engine + chrome (arrows, dots, header, autoplay, layout). The component no longer ships an items/variant API or any slide CSS. - Extract the homepage text+image+overlay slide into Landing/DashboardSlide (now owns the CarouselItem type). - Extract the use-case image+caption slide into UseCase/DashboardImageSlide. - Drop the items-mode render block and the CarouselItem export. - Rename dotCount -> slideCount (SSR dot count). - Replace the implicit carousel-slotted bleed with an explicit `bleed` prop. Blog and advantages carousels were already slot-based; they opt into `bleed` for their bordered cards. No visual change on any surface (check/lint/dev-parity/adversarial parity review all clean).
The Astro View Transitions router is not enabled anywhere on this site
(no <ClientRouter/>, no transition:* directives, no experimental flags;
Starlight 0.39.2 does not inject it), so astro:page-load never fires.
Every document.addEventListener('astro:page-load', ...) registration was
therefore dead code.
Remove all 23 registrations across 26 files and trim the now-inaccurate
'fires on every view-transition' comments. Each component keeps its real
init path (DOMContentLoaded listener, immediate init() call, or readyState
guard) and all idempotency guards, so behavior is unchanged today.
Verified: astro check 0 errors/warnings, eslint clean, zero astro:page-load
references remain.
rusikv
changed the title
2 participants
Summary
Restores the use-case dashboard carousels lost in the content migration, then unifies every stepped (embla) carousel on the site onto a single content-agnostic shared
Carouselcomponent, fixes the accessibility/landing issues found along the way, and removes the deadastro:page-loadlisteners that were scattered site-wide.What's included
Use-case dashboard carousels — restored the dashboard-screenshot carousels (and their images) on air-quality-monitoring, scada-energy-management, smart-metering, smart-retail, and water-metering.
Shared
Carouselcomponent (src/components/Carousel/)Carousel.astrois a slot-only, content-agnostic engine — embla wiring + chrome (arrows, dots, header, autoplay) and nothing about slide content. Props:ariaLabel,loop,autoplay,autoplayInterval,layout: 'single' | 'multi',showArrows,arrowPlacement,title,showDots,slideCount(SSR dots),bleed,class.CarouselSlide.astro(slot wrapper with SSRaria-label="N of M") andcarousel-init.ts(lazy embla load viaIntersectionObserver,prefers-reduced-motiongate, dot/arrow wiring,carousel:selectevent for future master-detail consumers).Landing/DashboardSlide.astro(homepage text + image + overlay slide; owns theCarouselItemtype) andUseCase/DashboardImageSlide.astro(use-case image + caption slide).UseCaseCarouselwrapper and the oldLanding/Carousel.astro;AdvantagesCarouselshrank356→190 lines; the blog dropped its own embla wiring.Carousel behavior/polish
embla-carousel-autoplayplugin; respects reduced motion; stops on touch swipe but keeps desktop hover-resume.region/slideroles, dotaria-current); removedMath.random()ids.Blog
slideCount, full-bleed on mobile via thebleedprop).Marquee accessibility (logo strip, company hero photos, visualization widgets strip)
prefers-reduced-motionguards andaria-hiddenon the duplicated copies (they previously animated unconditionally and announced content twice to screen readers).Site-wide
astro:page-loadcleanup<ClientRouter/>, notransition:*directives, noexperimentalflags; Starlight 0.39.2 doesn't inject it), soastro:page-loadnever fires. Everydocument.addEventListener('astro:page-load', …)registration was dead code.DOMContentLoadedlistener, an immediateinit()call, or areadyStateguard) and all idempotency guards, so behavior is unchanged today.Intended behavior changes (call-outs for review)
astro:page-loadremoval: purely removes dead code today. Trade-off: if Starlight view transitions are ever enabled, these components would need re-init wired back (via a shared helper) to keep working across client-side navigations.Out of scope / deliberate non-changes
Trainings/CoursesSection(a master-detail controller, not a plain slider) is kept separate; it can adoptcarousel-init.ts+carousel:selectlater.Marqueeprimitive: the three marquees diverge too much and Astro renders a<slot/>only once (so a generic primitive can't produce the duplicated copy the seamless loop needs). Their accessibility was fixed in place instead.Verification
astro check: 0 errors / 0 warnings (553 files).eslint: clean.astro:page-loadreferences remain anywhere insrc/./blog/, the advantages strip, and all three marquees.