Add support for cross-page anchor links#160
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
6e46e02 to
7b63bba
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
60002f0 to
ee78408
Compare
This comment was marked as resolved.
This comment was marked as resolved.
|
I've updated the test-PR, so the latest version of this PR can now be tested at https://lundalogik.github.io/lime-elements/versions/PR-3843/ Why not try this deep-link to the Reduced Presence example for limel-button? |
70dc1a1 to
3e7810d
Compare
This adds two features to enable cross-page anchor links: 1. Auto-generate heading IDs using rehype-slug - All headings (h1-h6) now get an id attribute based on their text - IDs are URL-safe slugs (e.g., 'Getting Started' → 'getting-started') 2. Scroll to anchor after content renders - After markdown content is rendered, check for URL hash - If an anchor is present, scroll the matching element into view - Also handles hash changes for same-page navigation This enables links like /guide/changelog#v2-features to work correctly in the SPA, where content loads asynchronously.
3e7810d to
b680725
Compare
📝 WalkthroughWalkthroughThis PR adds heading slug generation and anchor scrolling: it installs rehype-slug, integrates slugging into the markdown pipeline, introduces shadow-DOM-aware anchor scroll utilities, and wires hashchange-driven scrolling into Markdown and component lifecycle hooks. Changes
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Markdown as Markdown Component
participant Anchor as AnchorScroll Utils
participant Shadow as Shadow DOM
User->>Browser: Click link or change hash
Browser->>Markdown: emit "hashchange"
Markdown->>Markdown: handleHashChange / render
Markdown->>Anchor: scrollToAnchor(shadowRoot)
Anchor->>Anchor: getAnchorId() parse hash
Anchor->>Browser: requestAnimationFrame
Browser->>Shadow: querySelector by id in shadowRoot
Shadow->>Shadow: element.scrollIntoView()
Shadow->>User: view jumps to anchor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/components/component/component.tsx`:
- Around line 65-77: Route/hash fragments (e.g., `#/component/x`#heading) are
being passed through getRoute() into scroll targets and IDs, causing mismatches;
update usages in componentDidLoad, componentDidUpdate, and handleRouteChange to
strip any anchor fragment from the string returned by getRoute() before
assigning to scrollToOnNextUpdate or passing to scrollToElement (use a simple
split on '#' or URL parsing), so scrollToElement(this.host.shadowRoot, ...) and
assignments to this.scrollToOnNextUpdate always receive route-only values
without trailing fragments.
In `@src/components/markdown/markdown.tsx`:
- Around line 49-56: renderMarkdown() can produce stale UI because
markdownToHtml(this.text) is awaited and older calls may finish after newer
ones; fix by capturing a render token or snapshot of this.text before awaiting
and aborting update if it no longer matches the latest. Concretely, add a
monotonic renderId or store a local const currentText = this.text at the start
of renderMarkdown(), incrementing or comparing against a this._latestRenderId,
then after awaiting markdownToHtml(this.text, types) verify the
token/currentText still matches this._latestRenderId/this.text; only then assign
to host.shadowRoot.querySelector('#root').innerHTML and call
scrollToAnchor(this.host.shadowRoot). Ensure renderId/_latestRenderId is updated
whenever text changes so outdated async results are ignored.
Extract common anchor scrolling functionality into anchor-scroll.ts: - getRoute(): get route from hash for hash-based routing - getAnchorId(): extract anchor fragment from URL - scrollToAnchor(): scroll to anchor in shadow root - scrollToElement(): scroll to element by ID in shadow root Both kompendium-markdown and kompendium-component now use this shared utility instead of duplicating the logic.
b680725 to
0716316
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/component/component.tsx`:
- Around line 78-80: handleRouteChange sets the private field
scrollToOnNextUpdate but because that field isn't reactive the component won't
re-render and componentDidUpdate won't run; either mark scrollToOnNextUpdate
with the `@State`() decorator (so assignments inside handleRouteChange trigger a
re-render) or call this.forceUpdate() at the end of handleRouteChange so
componentDidUpdate (and the scroll logic) executes—update the declaration of
scrollToOnNextUpdate or add the forceUpdate call in handleRouteChange
accordingly (refer to scrollToOnNextUpdate, handleRouteChange, and
componentDidUpdate).
🧹 Nitpick comments (2)
src/components/markdown/markdown.tsx (2)
43-45:componentDidUpdatetriggersrenderMarkdownunconditionally on every update.This will re-render markdown even when unrelated state changes occur. Consider adding a check to only re-render when
this.textactually changes, or rely on Stencil's@Watchdecorator for thetextprop instead.♻️ Suggested approach using `@Watch`
+import { Component, h, Prop, Element, Watch } from '@stencil/core'; -import { Component, h, Prop, Element } from '@stencil/core'; ... - protected componentDidUpdate(): void { - this.renderMarkdown(); - } + `@Watch`('text') + protected onTextChange(): void { + this.renderMarkdown(); + }
62-66: Potential null reference if#rootelement is not found.If
querySelector('#root')returnsnull, setting.innerHTMLwill throw. While unlikely given the component'srender()method, a defensive check would be safer.🛡️ Optional defensive check
- this.host.shadowRoot.querySelector('#root').innerHTML = - file?.toString(); + const root = this.host.shadowRoot.querySelector('#root'); + if (root) { + root.innerHTML = file?.toString(); + }
adrianschmidt
left a comment
There was a problem hiding this comment.
This seems to work as expected. The anchor links from the navigation menu to Properties, Events, Methods, and Styles are not perfect. On long pages they they don't scroll the page far enough down. But it's exactly the same on main, and the new anchor support in the markdown component works perfectly as far as I've been able to test.
|
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
This PR adds support for cross-page anchor links in kompendium documentation, addressing the issue where anchor links don't work due to asynchronous page loading in the SPA.
Changes
1. Auto-generate heading IDs (rehype-slug)
All headings (h1-h6) in markdown content now automatically get an
idattribute based on their text content:# Getting Started→<h1 id="getting-started">Getting Started</h1>## What's New in v2.0?→<h2 id="whats-new-in-v20">What's New in v2.0?</h2>2. Scroll to anchor after content renders
The
kompendium-markdowncomponent now:hashchangeevents for same-page anchor navigationUsage
This enables links like:
/guide/changelog#v2-features/guide/getting-started#installationto work correctly even though content loads asynchronously.
Related
fix: #60
Summary by CodeRabbit
New Features
Tests