-
-
Notifications
You must be signed in to change notification settings - Fork 271
fix: enable hash link scrolling in README #1013
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
24613b0
b9d138f
99ebc08
53d504b
b732bee
43a7035
5c167df
8a87db7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,4 +1,6 @@ | ||||||||||||||||||||||||||||||||
| <script setup lang="ts"> | ||||||||||||||||||||||||||||||||
| import { scrollToAnchor } from '~/utils/scrollToAnchor' | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| defineProps<{ | ||||||||||||||||||||||||||||||||
| html: string | ||||||||||||||||||||||||||||||||
| }>() | ||||||||||||||||||||||||||||||||
|
|
@@ -9,6 +11,7 @@ const { copy } = useClipboard() | |||||||||||||||||||||||||||||||
| // Combined click handler for: | ||||||||||||||||||||||||||||||||
| // 1. Intercepting npmjs.com links to route internally | ||||||||||||||||||||||||||||||||
| // 2. Copy button functionality for code blocks | ||||||||||||||||||||||||||||||||
| // 3. Smooth scrolling for hash links | ||||||||||||||||||||||||||||||||
| function handleClick(event: MouseEvent) { | ||||||||||||||||||||||||||||||||
| const target = event.target as HTMLElement | undefined | ||||||||||||||||||||||||||||||||
| if (!target) return | ||||||||||||||||||||||||||||||||
|
|
@@ -40,13 +43,24 @@ function handleClick(event: MouseEvent) { | |||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Handle npmjs.com link clicks - route internally | ||||||||||||||||||||||||||||||||
| // Handle anchor link clicks | ||||||||||||||||||||||||||||||||
| const anchor = target.closest('a') | ||||||||||||||||||||||||||||||||
| if (!anchor) return | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| const href = anchor.getAttribute('href') | ||||||||||||||||||||||||||||||||
| if (!href) return | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Handle hash links with smooth scrolling (case is normalized server-side) | ||||||||||||||||||||||||||||||||
| if (href.startsWith('#')) { | ||||||||||||||||||||||||||||||||
| event.preventDefault() | ||||||||||||||||||||||||||||||||
| const id = href.slice(1) | ||||||||||||||||||||||||||||||||
| if (id) { | ||||||||||||||||||||||||||||||||
| scrollToAnchor(id) | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
54
to
61
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would it work if we just returned, deferring to the browser?
Suggested change
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nope, different case-registry, we need to modify href (toLowerCase())
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but why? we're generating the ids and the links. if case matters, surely we should do npmx.dev/server/utils/readme.ts Line 321 in 0531e3f
npmx.dev/server/utils/readme.ts Lines 192 to 195 in 0531e3f
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Regarding the previous comment - sorry, I meant that this alone is not enough - if we modify the link, it will work
Better
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| // Handle npmjs.com link clicks - route internally | ||||||||||||||||||||||||||||||||
| const match = href.match(/^(?:https?:\/\/)?(?:www\.)?npmjs\.(?:com|org)(\/.+)$/) | ||||||||||||||||||||||||||||||||
| if (!match || !match[1]) return | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -190,8 +190,9 @@ function slugify(text: string): string { | |
| function resolveUrl(url: string, packageName: string, repoInfo?: RepositoryInfo): string { | ||
| if (!url) return url | ||
| if (url.startsWith('#')) { | ||
| // Prefix anchor links to match heading IDs (avoids collision with page IDs) | ||
| return `#user-content-${url.slice(1)}` | ||
| // Prefix anchor links and lowercase to match heading IDs | ||
| // (slugify uses toLowerCase, and prefix avoids collision with page IDs) | ||
| return `#user-content-${url.slice(1).toLowerCase()}` | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| if (hasProtocol(url, { acceptRelative: true })) { | ||
| try { | ||
|
|
@@ -265,9 +266,9 @@ function resolveImageUrl(url: string, packageName: string, repoInfo?: Repository | |
| } | ||
|
|
||
| // Helper to prefix id attributes with 'user-content-' | ||
| function prefixId(tagName: string, attribs: sanitizeHtml.Attributes) { | ||
| const prefixId = (tagName: string, attribs: sanitizeHtml.Attributes) => { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @niveshdandyan Why moving to a var with anonymous fn ? 🤔 |
||
| if (attribs.id && !attribs.id.startsWith('user-content-')) { | ||
| attribs.id = `user-content-${attribs.id}` | ||
| attribs.id = `user-content-${attribs.id.toLowerCase()}` | ||
| } | ||
| return { tagName, attribs } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,30 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { mountSuspended } from '@nuxt/test-utils/runtime' | ||
| import Readme from '~/components/Readme.vue' | ||
|
|
||
| describe('Readme', () => { | ||
| describe('rendering', () => { | ||
| it('renders the provided HTML content', async () => { | ||
| const component = await mountSuspended(Readme, { | ||
| props: { html: '<p>Hello world</p>' }, | ||
| }) | ||
| expect(component.html()).toContain('Hello world') | ||
| }) | ||
| }) | ||
|
|
||
| describe('hash link click handling', () => { | ||
| it('allows native browser handling for hash links (does not prevent default)', async () => { | ||
| const component = await mountSuspended(Readme, { | ||
| props: { html: '<a href="#Installation">Installation</a>' }, | ||
| }) | ||
|
|
||
| const link = component.find('a') | ||
| const clickEvent = new MouseEvent('click', { bubbles: true, cancelable: true }) | ||
| link.element.dispatchEvent(clickEvent) | ||
|
|
||
| // Hash links should NOT have default prevented - browser handles them natively | ||
| // (Case normalization happens server-side when generating the href) | ||
| expect(clickEvent.defaultPrevented).toBe(false) | ||
| }) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' | ||
| import { scrollToAnchor } from '~/utils/scrollToAnchor' | ||
|
|
||
| describe('scrollToAnchor', () => { | ||
| let scrollToSpy: ReturnType<typeof vi.fn> | ||
| let replaceStateSpy: ReturnType<typeof vi.fn> | ||
| let testElement: HTMLElement | ||
|
|
||
| beforeEach(() => { | ||
| // Spy on window.scrollTo | ||
| scrollToSpy = vi.fn() | ||
| vi.stubGlobal('scrollTo', scrollToSpy) | ||
|
|
||
| // Spy on history.replaceState | ||
| replaceStateSpy = vi.spyOn(history, 'replaceState').mockImplementation(() => {}) | ||
|
|
||
| // Create a test element | ||
| testElement = document.createElement('div') | ||
| testElement.id = 'test-section' | ||
| document.body.appendChild(testElement) | ||
| }) | ||
|
|
||
| afterEach(() => { | ||
| vi.restoreAllMocks() | ||
| vi.unstubAllGlobals() | ||
| // Clean up test element | ||
| if (testElement && testElement.parentNode) { | ||
| testElement.parentNode.removeChild(testElement) | ||
| } | ||
| }) | ||
|
|
||
| describe('with custom scrollFn', () => { | ||
| it('calls the provided scroll function with the id', () => { | ||
| const scrollFn = vi.fn() | ||
| scrollToAnchor('test-section', { scrollFn }) | ||
|
|
||
| expect(scrollFn).toHaveBeenCalledWith('test-section') | ||
| expect(scrollToSpy).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('does not update URL when using custom scrollFn', () => { | ||
| const scrollFn = vi.fn() | ||
| scrollToAnchor('test-section', { scrollFn }) | ||
|
|
||
| expect(replaceStateSpy).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
||
| describe('with default scroll behavior', () => { | ||
| it('does nothing when element is not found', () => { | ||
| scrollToAnchor('non-existent-id') | ||
|
|
||
| expect(scrollToSpy).not.toHaveBeenCalled() | ||
| expect(replaceStateSpy).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('scrolls to element with smooth behavior', () => { | ||
| scrollToAnchor('test-section') | ||
|
|
||
| expect(scrollToSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| behavior: 'smooth', | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it('calculates scroll position with header offset', () => { | ||
| scrollToAnchor('test-section') | ||
|
|
||
| // Verify scrollTo was called with a top property (exact value depends on element position) | ||
| expect(scrollToSpy).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| top: expect.any(Number), | ||
| }), | ||
| ) | ||
| }) | ||
|
|
||
| it('updates URL hash by default', () => { | ||
| scrollToAnchor('test-section') | ||
|
|
||
| expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '#test-section') | ||
| }) | ||
|
|
||
| it('does not update URL when updateUrl is false', () => { | ||
| scrollToAnchor('test-section', { updateUrl: false }) | ||
|
|
||
| expect(scrollToSpy).toHaveBeenCalled() | ||
| expect(replaceStateSpy).not.toHaveBeenCalled() | ||
| }) | ||
| }) | ||
|
|
||
| describe('edge cases', () => { | ||
| it('handles empty id string without errors', () => { | ||
| expect(() => scrollToAnchor('')).not.toThrow() | ||
| expect(scrollToSpy).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| it('handles id with user-content prefix (GitHub-style anchors)', () => { | ||
| const userContentElement = document.createElement('div') | ||
| userContentElement.id = 'user-content-installation' | ||
| document.body.appendChild(userContentElement) | ||
|
|
||
| scrollToAnchor('user-content-installation') | ||
|
|
||
| expect(scrollToSpy).toHaveBeenCalled() | ||
| expect(replaceStateSpy).toHaveBeenCalledWith(null, '', '#user-content-installation') | ||
|
|
||
| document.body.removeChild(userContentElement) | ||
| }) | ||
| }) | ||
| }) |

Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that anchor link can have different case values, but the heading ID only uses lowercase. Both the anchor and the heading id (already so) need to be converted to lowercase
The current logic will not work after a reload or with link sharing and is generally redundant - it is always better to trust native capabilities as much as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback @alexdln! You're absolutely right about the case sensitivity issue.
I've addressed this in commit b9d138f by adding
.toLowerCase()when extracting the ID from the hash link. This ensures that links like#Installationwill correctly match heading IDs likeuser-content-installation(which are generated using theslugifyfunction that lowercases the text).The fix now follows the same pattern as the server-side
slugifyfunction which converts heading text to lowercase before creating the ID.I also added tests for the Readme component to verify the hash link handling and case sensitivity fix.