Skip to content

Commit 82c0c0a

Browse files
committed
Refactor Testimonials and Carousel to LitElement base class from HTMLElement, extract common embla code out of both into shared library
1 parent 9696f0e commit 82c0c0a

12 files changed

Lines changed: 1245 additions & 1080 deletions

File tree

_LIT_AND_CAROUSEL_REFACTOR.md

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,76 @@
1+
1. Issues with refactoring Carousel and Testimonials to LitElement
2+
No real blockers. The migration is straightforward because:
3+
4+
Light DOM is already solved. ThemePicker demonstrates the pattern: createRenderRoot() { return this }. Since all three components render their HTML via Astro templates (not Lit's render()), this is the correct approach and already in use.
5+
LitElement is already bundled. ThemePicker pulls it in, so there's no added dependency cost.
6+
Lifecycle maps cleanly. The manual connectedCallback → initialize() pattern in Carousel/Testimonials translates directly. The main changes are calling super.connectedCallback() / super.disconnectedCallback() and optionally using firstUpdated() instead of the manual DOMContentLoaded check.
7+
Nanostore integration improves. Carousel and Testimonials currently call createAnimationController manually. With LitElement they could use StoreController for reactive bindings (like ThemePicker does with createThemeController), though it's not required since the animation lifecycle uses callbacks rather than direct store subscriptions.
8+
The only thing to watch: the transition:persist directive must remain on the HTML custom element tag itself, not on an Astro component wrapper. That's already a project rule and isn't affected by LitElement vs HTMLElement.
9+
10+
2. Revised recommendation
11+
12+
With all three components sharing LitElement as a base, an abstract base class becomes the cleanest option.
13+
14+
Proposed structure:
15+
16+
```bash
17+
src/components/scripts/embla/
18+
EmblaCarouselBase.ts – abstract LitElement base class
19+
types.ts – shared types (TimerHandle, EmblaRootElement, etc.)
20+
logging.ts – createE2ELogger(prefix)
21+
```
22+
23+
## What goes in EmblaCarouselBase extends LitElement
24+
25+
All the code that's currently duplicated (~400 lines between Carousel and Testimonials):
26+
27+
Responsibility Implementation
28+
Light DOM createRenderRoot() { return this }
29+
Embla init/destroy initEmbla() / teardownEmbla() with options from abstract getter
30+
Autoplay plugin Setup, pending-state queue, scheduleAutoplayReady(), flushPendingAutoplayState()
31+
pause() / resume() public API Identical in both today
32+
Viewport observer setupViewportObserver() / teardownViewportObserver() / syncAutoplayWithViewport()
33+
Animation lifecycle registerAnimationLifecycle() wrapping createAnimationController
34+
Focus-visible pause handleFocusIn() / scheduleFocusVisiblePauseSync() / syncFocusVisiblePauseState()
35+
Prev/next buttons setupNavigationButtons() with enable/disable state updates
36+
Dots navigation setupDotsNavigation() with an overridable method for dot active styling
37+
E2E logging logForE2E() using a configurable prefix
38+
data-carousel-ready / data-carousel-autoplay attributes Set/remove in base
39+
connectedCallback / disconnectedCallback / teardown Full lifecycle orchestration
40+
41+
### What subclasses configure
42+
43+
Via abstract getters or overridable methods — keeping each subclass to ~50–100 lines of glue:
44+
45+
| Configuration point | Carousel | Testimonials | Skills (new) |
46+
| ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ |
47+
| `abstract get emblaOptions()` | [{ loop: true, align: 'start' }](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [{ loop: true, align: 'center' }](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [{ loop: true, align: 'start', dragFree: true }](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) |
48+
| `abstract get autoplayOptions()` | [{ delay: 4000, ... }](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [{ delay: 6000, ... }](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | `null` (no autoplay initially) |
49+
| [abstract get animationId()](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | `'carousel'` | ['testimonials-carousel'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | `'skills-carousel'` |
50+
| [abstract get scriptName()](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | ['CarouselElement'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | ['TestimonialsCarouselElement'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | `'SkillsCarouselElement'` |
51+
| `abstract get logPrefix()` | `'carousel'` | ['testimonials'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | `'skills'` |
52+
| `abstract get focusVisibleSelector()` | `'[data-carousel-slide] :focus-visible'` | `'[data-testimonials-slide] :focus-visible'` | component-specific |
53+
| `abstract queryElements()` | Uses Carousel selectors | Uses Testimonials selectors | Uses Skills selectors |
54+
55+
Then each subclass adds only its unique features:
56+
57+
| Feature | Where it lives |
58+
| ------------------------------------------------------------ | ------------------------------------------------------------ |
59+
| Keyboard nav (ArrowLeft/Right) | Carousel subclass only |
60+
| Status region ("Slide X of Y") | Carousel subclass only |
61+
| Autoplay toggle button + icon swap | Testimonials subclass only |
62+
| Viewport ID / `aria-controls` on buttons | Testimonials subclass only |
63+
| Dot active styling ([is-active](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) vs `w-6`) | Override `applyDotActiveStyle()` / `applyDotInactiveStyle()` |
64+
65+
Two reasonable paths:
66+
67+
A: ThemePicker stays independent but extracts just the nav-button wiring into a small utility function in src/components/scripts/embla/navigation.ts (~30 lines). This avoids forcing ThemePicker into a base class that's 90% irrelevant to it.
68+
B: ThemePicker extends EmblaCarouselBase with autoplay disabled and most features no-op'd. This feels forced and adds cognitive overhead for no real benefit.
69+
I'd recommend A. The base class serves the autoplay-carousel family (Carousel, Testimonials, Skills, and any future autoplay carousel). ThemePicker shares one small utility.
70+
71+
Estimated result
72+
EmblaCarouselBase.ts: ~350–400 lines (extracted from current duplication)
73+
Each subclass (Carousel, Testimonials, Skills): ~50–150 lines of component-specific code
74+
navigation.ts utility for ThemePicker: ~30 lines
75+
Net reduction: ~400+ lines eliminated across Carousel and Testimonials
76+
Skills starts with a thin subclass instead of copy-pasting a fourth time

_TODO.md

Lines changed: 143 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,45 @@ Cons: Browser rendering can be inconsistent across different operating systems a
2121

2222
Axe accessibility (2) - axe-core integration
2323

24+
## Missing E2E Component Tests
25+
26+
- Code/CodeBlock
27+
- Code/CodeTabs
28+
- Consent/Checkbox
29+
30+
## Testimonials on mobile
31+
32+
We have E2E errors again testimonials slide on mobile chrome and safari. I think the problem is that we are pausing carousels when part of the carousel is outside of the viewport, and the testimonials are too large to display on mobile without being off viewport.
33+
34+
`test/e2e/specs/04-components/testimonials.spec.ts`:244:3 › Testimonials Component › @ready testimonials auto-rotate changes slide index
35+
36+
## Refactor of Common Code in Carousel related components
37+
38+
See if there's any code in common between Carousel, Testimonials, and Themepicker. We have another one to add that uses the carousel code - for Skills.
39+
40+
Issue identified with refactoring Testimonial and Carousel to Lit custom web components:
41+
42+
The only thing to watch: the `transition:persist` directive must remain on the HTML custom element tag itself, not on an Astro component wrapper. That's already a project rule and isn't affected by `LitElement` vs `HTMLElement`.
43+
44+
#### Selector files
45+
46+
Carousel/client/selectors.ts and Testimonials/client/selectors.ts follow an identical pattern — only the CSS selectors and data-attribute names differ. The query/get function pairs are structurally identical.
47+
48+
### What's genuinely different
49+
50+
| Concern | Carousel | Testimonials | ThemePicker |
51+
| -------------------------------- | ------------------------------------------------------------ | ------------------------------------------------------------ | ------------------------------------------------------------ |
52+
| Embla options | [align: 'start'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [align: 'center'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [align: 'center'](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html), [containScroll](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html), no loop |
53+
| Autoplay delay | 4000ms | 6000ms | None |
54+
| Autoplay toggle button || ✅ (play/pause icon swap) ||
55+
| Keyboard nav (ArrowLeft/Right) ||||
56+
| Status region (`"Slide X of Y"`) ||||
57+
| Dot styling | [is-active](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) class | width-based (`w-3``w-6`) | None |
58+
| Viewport ID / `aria-controls` ||||
59+
| View Transitions integration ||| ✅ (Lit + `astro:after-swap`) |
60+
| Tooltip portal ||||
61+
| Base class | [HTMLElement](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [HTMLElement](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) | [LitElement](vscode-file://vscode-app/usr/share/code/resources/app/out/vs/code/electron-browser/workbench/workbench.html) |
62+
2463
## Youtube video for Backstage IDP hero on Home page
2564

2665
Discuss agentic AI integrations to add to Backstage
@@ -39,14 +78,6 @@ Vercel AI Gateway, maybe could use for a chatbot:
3978

4079
https://vercel.com/kevin-browns-projects-dd474f73/astro-webstackbuilders-com/ai-gateway
4180

42-
## Testimonials on mobile
43-
44-
We have E2E errors again testimonials slide on mobile chrome and safari. I think the problem is that we are pausing carousels when part of the carousel is outside of the viewport, and the testimonials are too large to display on mobile without being off viewport.
45-
46-
See if there's any code in common between Carousel, Testimonials, and Themepicker. We have another one to add that uses the carousel code - for Skills.
47-
48-
`test/e2e/specs/04-components/testimonials.spec.ts`:244:3 › Testimonials Component › @ready testimonials auto-rotate changes slide index
49-
5081
## Move containers to dev server from Playwright
5182

5283
We should start the mock containers with the dev server instead of with Playwright so that they're useable in a dev environment.
@@ -238,3 +269,107 @@ This article has different approaches to [print pagination](https://www.customjs
238269
- cover.jpg for reliability-and-testing needs touch up in GIMP
239270
- We need to check for short form and deep article articles where the deep-dive index.pdf has a non-featured tag lik "argo-cd" only in the pdf.mdx. In those cases, we should make sure the callout for the deep dive includes the name of that non-featured (technology) tag and add the name to the tags: frontmatter key in the index.mdx
240271
- Need an article on OpenStack
272+
273+
## HTMLElement vs. extends HTMLElement
274+
275+
A bunch of our web components extend directly from HTMLElement instead of following the instructions to extend LitElement.
276+
277+
- Code/CodeBlock
278+
- Code/CodeTabs
279+
- Consent/Banner
280+
- Consent/Checkbox
281+
- Consent/Preferences
282+
283+
And the embla components: Carousel and Testimonials
284+
285+
The WebComponentModule type in this file is used throughout component scripts:
286+
287+
`src/components/scripts/@types/webComponentModule.ts`
288+
289+
There are testing fixtures that might be targeting HTMLElement instead of LitElement:
290+
291+
`isLikelyWebComponent()` in `test/eslint/enforce-centralized-events-rule.ts`
292+
line 113 in `test/eslint/__tests__/enforce-centralized-events-rule.spec.ts`
293+
294+
interface `ElementWithTestProperties` in `test/e2e/assertions/index.ts`
295+
296+
/home/kevin/Repos/WebstackBuilders/CorporateWebsite/astro.webstackbuilders.com/src/components/Carousel/client/index.ts
297+
70:5 error Use `keyup with addButtonEventListeners or addWrapperEventListeners` from `elementListeners` for keyboard events (use keyup instead of keydown for better accessibility) instead of direct addEventListener. This ensures consistent accessibility support (isComposing check, repeat prevention, Enter/Escape key handling) custom-rules/enforce-centralized-events
298+
299+
✖ 1 problem (1 error, 0 warnings)
300+
301+
302+
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯ Failed Tests 3 ⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
303+
304+
FAIL src/components/Testimonials/client/__tests__/index.spec.ts > Testimonials component > registers the web component and generates pagination dots
305+
AssertionError: expected null to be 'testimonials-3-viewport' // Object.is equality
306+
307+
- Expected:
308+
"testimonials-3-viewport"
309+
310+
+ Received:
311+
null
312+
313+
❯ src/components/Testimonials/client/__tests__/index.spec.ts:198:51
314+
196|
315+
197| dots.forEach(dot => {
316+
198| expect(dot.getAttribute('aria-controls')).toBe(viewportId)
317+
| ^
318+
199| })
319+
200| expect(prevBtn?.getAttribute('aria-controls')).toBe(viewportId)
320+
❯ src/components/Testimonials/client/__tests__/index.spec.ts:197:12
321+
❯ assert src/components/Testimonials/client/__tests__/index.spec.ts:114:13
322+
❯ assert test/unit/helpers/litRuntime.ts:308:10
323+
❯ test/unit/helpers/litRuntime.ts:246:9
324+
❯ withJsdomEnvironment test/unit/helpers/litRuntime.ts:147:10
325+
❯ renderInJsdom test/unit/helpers/litRuntime.ts:218:2
326+
❯ Module.executeRender test/unit/helpers/litRuntime.ts:320:2
327+
328+
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[1/3]
329+
330+
FAIL src/components/Home/Hero/client/__tests__/index.spec.ts > HomeHeroElement (Lit) > types the ready prompt one character every 500ms and stops when complete
331+
AssertionError: expected '' to be 'r' // Object.is equality
332+
333+
- Expected
334+
+ Received
335+
336+
- r
337+
338+
❯ src/components/Home/Hero/client/__tests__/index.spec.ts:57:38
339+
55|
340+
56| vi.advanceTimersByTime(STEP_MS)
341+
57| expect(readyText?.textContent).toBe('r')
342+
| ^
343+
58|
344+
59| vi.advanceTimersByTime(STEP_MS)
345+
❯ withJsdomEnvironment test/unit/helpers/litRuntime.ts:147:10
346+
❯ src/components/Home/Hero/client/__tests__/index.spec.ts:39:5
347+
348+
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯[2/3]
349+
350+
FAIL src/components/Home/Hero/client/__tests__/index.spec.ts > HomeHeroElement (Lit) > skips animation and shows final text when reduced motion is preferred
351+
AssertionError: expected '' to be 'heck yes, let\'s talk...' // Object.is equality
352+
353+
- Expected
354+
+ Received
355+
356+
- heck yes, let's talk...
357+
358+
❯ src/components/Home/Hero/client/__tests__/index.spec.ts:90:38
359+
88| const readyText = window.document.querySelector<HTMLElement>('[data-hero-ready-text]')
360+
89| expect(readyText).toBeTruthy()
361+
90| expect(readyText?.textContent).toBe(READY_TEXT)
362+
| ^
363+
91|
364+
92| vi.advanceTimersByTime(STEP_MS * READY_TEXT.length)
365+
❯ withJsdomEnvironment test/unit/helpers/litRuntime.ts:147:10
366+
❯ src/components/Home/Hero/client/__tests__/index.spec.ts:75:5
367+
368+
369+
02:58:50 [ERROR] [vite] ✗ Build failed in 4.72s
370+
[@mdx-js/rollup] Cannot assign to read only property 'name' of object 'BuildError: Mermaid couldn't graph this diagram.'
371+
file: /home/kevin/Repos/WebstackBuilders/CorporateWebsite/astro.webstackbuilders.com/src/content/articles/consumer-driven-contract-testing-pact-internal-apis/index.mdx
372+
Stack trace:
373+
at Object.transform (file:///home/kevin/Repos/WebstackBuilders/CorporateWebsite/astro.webstackbuilders.com/node_modules/@astrojs/mdx/dist/vite-plugin-mdx.js:60:18)
374+
at process.processImmediate (node:internal/timers:473:9)
375+
at async ModuleLoader.addModuleSource (file:///home/kevin/Repos/WebstackBuilders/CorporateWebsite/astro.webstackbuilders.com/node_modules/rollup/dist/es/shared/node-entry.js:21363:36)

0 commit comments

Comments
 (0)