-
-
Notifications
You must be signed in to change notification settings - Fork 237
feat: use native scroll anchoring #1209
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
eb99701 to
1fba9f2
Compare
1fba9f2 to
798ab1d
Compare
798ab1d to
486a12a
Compare
📝 WalkthroughWalkthroughRemoved the scrollToAnchor utility and all explicit scroll-to-heading logic. Simplified useActiveTocItem to return only { activeId } and removed its scroll-related APIs and cleanup. Updated README/TOC components and section headings to use NuxtLink/LinkBase for in-page anchors; Readme.vue now intercepts relative Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/ReadmeTocDropdown.vue (1)
128-135:⚠️ Potential issue | 🟠 MajorKeyboard navigation may not navigate to the selected item.
When pressing Enter,
select()is called which closes the dropdown and focuses the trigger, but the actual navigation to the highlighted TOC item doesn't occur. TheNuxtLinkcomponents handle click-based navigation, but keyboard selection bypasses this.Consider programmatically navigating to the highlighted item's anchor when Enter is pressed.
🐛 Proposed fix
case 'Enter': { event.preventDefault() const item = props.toc[highlightedIndex.value] if (item) { + navigateTo(`#${item.id}`) select() } break }
🧹 Nitpick comments (1)
app/components/CollapsibleSection.vue (1)
85-92: Button focus-visible styling may conflict with global rule.The button uses inline
focus-visible:outline-accent/70(line 88), but the project has a global focus-visible rule for buttons inmain.css. Per the project's coding conventions, buttons should rely on the global rule rather than per-element utilities.♻️ Suggested fix to remove inline focus-visible utility
<button :id="buttonId" type="button" - class="w-4 h-4 flex items-center justify-center text-fg-subtle hover:text-fg-muted transition-colors duration-200 shrink-0 focus-visible:outline-accent/70 rounded" + class="w-4 h-4 flex items-center justify-center text-fg-subtle hover:text-fg-muted transition-colors duration-200 shrink-0 rounded" :aria-expanded="isOpen"Based on learnings: "In the npmx.dev project, focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes."
486a12a to
b0ea3e6
Compare
b0ea3e6 to
2573cd3
Compare
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.
Actionable comments posted: 1
186cc2b to
cbceb47
Compare
|
Hi! I'm I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:
|
cbceb47 to
595fc2f
Compare
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/ReadmeTocDropdown.vue (1)
99-134:⚠️ Potential issue | 🟠 MajorEnter key no longer activates the highlighted TOC link.
On Line 128, the Enter handler only closes the dropdown, so keyboard users cannot navigate to the highlighted heading. Trigger the corresponding link when Enter is pressed.
Suggested fix
case 'Enter': { event.preventDefault() const item = props.toc[highlightedIndex.value] if (item) { - select() + const el = document.getElementById( + `${listboxId}-${item.id}`, + ) as HTMLAnchorElement | null + if (el) { + el.click() + break + } } + select() break }
|
would you check the |
595fc2f to
2babad4
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
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.
Actionable comments posted: 1
2babad4 to
6584d73
Compare
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/ReadmeTocDropdown.vue (1)
99-133:⚠️ Potential issue | 🟠 MajorKeyboard activation regression + empty TOC guard missing.
Pressing Enter only closes the dropdown, so keyboard users can’t navigate to the highlighted item. Also, when
tocis empty, the Arrow key logic can hit modulo-by-zero and produceNaN. Consider routing the Enter action to the highlighted link and short‑circuit when there are no items.🛠️ Suggested fix
function handleKeydown(event: KeyboardEvent) { if (!isOpen.value) return const itemCount = props.toc.length + if (itemCount === 0) return switch (event.key) { case 'ArrowDown': event.preventDefault() highlightedIndex.value = (highlightedIndex.value + 1) % itemCount break case 'ArrowUp': event.preventDefault() highlightedIndex.value = highlightedIndex.value <= 0 ? itemCount - 1 : highlightedIndex.value - 1 break case 'Enter': { event.preventDefault() - const item = props.toc[highlightedIndex.value] - if (item) { - select() - } + if (highlightedIndex.value < 0 || highlightedIndex.value >= itemCount) return + const item = props.toc[highlightedIndex.value] + if (!item) return + const link = document.getElementById(`${listboxId}-${item.id}`) + if (link instanceof HTMLElement) { + link.click() + } else { + select() + } break }
|
@danielroe, fixed it. |
6584d73 to
d3460dc
Compare
|
this seems to have broken the mobile styling of the docs/code/compare buttons |
Unsure, I'm redeploying because the whole layout is broken again after the last deployment. |
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/composables/useActiveTocItem.ts (1)
44-79:⚠️ Potential issue | 🟠 MajorTrack all currently intersecting headings to prevent erratic TOC selection.
The callback only processes
entries(changed intersections), which misses headings already visible. When heading B becomes visible closer to the viewport top than already-visible heading A, the code setsactiveIdto B without checking A's position. Maintain state of all intersecting headings in aMap, updating it on each callback, and derive the active heading from the complete set.Proposed fix
let observer: IntersectionObserver | null = null const headingElements = new Map<string, Element>() + const intersecting = new Map<string, number>()// Create observer that triggers when headings cross the top 20% of viewport observer = new IntersectionObserver( entries => { - // Get all visible headings sorted by their position - const visibleHeadings: { id: string; top: number }[] = [] - for (const entry of entries) { if (entry.isIntersecting) { - visibleHeadings.push({ - id: entry.target.id, - top: entry.boundingClientRect.top, - }) + intersecting.set(entry.target.id, entry.boundingClientRect.top) + } else { + intersecting.delete(entry.target.id) } } + + const visibleHeadings = Array.from(intersecting, ([id, top]) => ({ id, top }))
🧹 Nitpick comments (1)
app/composables/useActiveTocItem.ts (1)
12-111: Consider splitting this composable into smaller helpers.
The function is ~100 lines, making it harder to maintain and reason about. Extracting helpers (e.g.,collectHeadingElements,computeActiveId) would keep it within the recommended size and improve clarity.As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
|
@danielroe, a redeployment fixed it?! |
Adapts anchor scrolling across the application.
PR title should maybe be:
fix: improve anchor scrollingor something.