feat: add sidebar toggle to package code browser#2352
feat: add sidebar toggle to package code browser#2352jfernsio wants to merge 5 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
|
Hello! Thank you for opening your first PR to npmx, @jfernsio! 🚀 Here’s what will happen next:
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughAdds a persisted sidebar collapse state to the package code viewer by introducing a reactive Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
389-390: Use global focus-visible styling for this button instead of inline utility classes.On Line 389,
focus-visible:outline-accent/70should be removed to follow the project’s shared button/select focus pattern.Suggested diff
- class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors focus-visible:outline-accent/70 rounded" + class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors rounded"Based on learnings, focus-visible styling for
button/selectshould be applied globally viaapp/assets/main.css, not per-element utility classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21d559b3-820f-46e9-91d7-648a9ccb385a
📒 Files selected for processing (2)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vuei18n/locales/en.json
| > | ||
| <div class="flex items-center gap-2"> | ||
| <!-- Sidebar toggle button --> | ||
| <button |
There was a problem hiding this comment.
Consider adding aria-expanded and aria-controls to this button
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
390-390: Use global focus-visible styling for this button.Please remove the inline
focus-visible:outline-accent/70utility from this<button>and rely on the global button focus-visible rule for consistency.Suggested change
- class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors focus-visible:outline-accent/70 rounded" + class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors rounded"Based on learnings: in this repo,
button:focus-visiblestyling is defined globally inapp/assets/main.css, and per-element inline focus-visible utilities on buttons/selects should not be used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: da83f4a1-c17f-4443-ae71-2a897b41efb9
📒 Files selected for processing (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue (1)
391-391: Avoid inlinefocus-visibleutility class on buttons.The project's global CSS applies
focus-visiblestyling for buttons and selects viamain.css. Usingfocus-visible:outline-accent/70inline here is inconsistent with the established pattern.Remove the inline utility and rely on the global rule for consistency.
♻️ Proposed fix
- class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors focus-visible:outline-accent/70 rounded" + class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors rounded"Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css... Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates."
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: daaff786-5c6f-44c5-89a4-67c588e76fe5
📒 Files selected for processing (1)
app/pages/package-code/[[org]]/[packageName]/v/[version]/[...filePath].vue
| <aside | ||
|
|
||
| id="file-tree-sidebar" | ||
| v-show="!isSidebarCollapsed" | ||
| class="w-64 lg:w-72 border-ie border-border shrink-0 hidden md:block bg-bg-subtle sticky top-25 self-start h-[calc(100vh-7rem)] overflow-y-auto" | ||
|
|
||
| class="sticky top-25 w-64 lg:w-72 hidden md:block h-[calc(100vh-10.5rem)] shrink-0 self-start bg-bg-subtle border-ie border-border" | ||
|
|
||
| > |
There was a problem hiding this comment.
Duplicate class attribute causes build failure.
The <aside> element has two class attributes (lines 366 and 368), which is invalid HTML and is causing the Storybook/Chromatic build to fail with "Duplicate attribute" error. You need to merge these into a single class attribute.
🐛 Proposed fix to merge the duplicate class attributes
<aside
id="file-tree-sidebar"
v-show="!isSidebarCollapsed"
- class="w-64 lg:w-72 border-ie border-border shrink-0 hidden md:block bg-bg-subtle sticky top-25 self-start h-[calc(100vh-7rem)] overflow-y-auto"
-
- class="sticky top-25 w-64 lg:w-72 hidden md:block h-[calc(100vh-10.5rem)] shrink-0 self-start bg-bg-subtle border-ie border-border"
-
+ class="sticky top-25 w-64 lg:w-72 hidden md:block h-[calc(100vh-10.5rem)] shrink-0 self-start bg-bg-subtle border-ie border-border overflow-y-auto"
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <aside | |
| id="file-tree-sidebar" | |
| v-show="!isSidebarCollapsed" | |
| class="w-64 lg:w-72 border-ie border-border shrink-0 hidden md:block bg-bg-subtle sticky top-25 self-start h-[calc(100vh-7rem)] overflow-y-auto" | |
| class="sticky top-25 w-64 lg:w-72 hidden md:block h-[calc(100vh-10.5rem)] shrink-0 self-start bg-bg-subtle border-ie border-border" | |
| > | |
| <aside | |
| id="file-tree-sidebar" | |
| v-show="!isSidebarCollapsed" | |
| class="sticky top-25 w-64 lg:w-72 hidden md:block h-[calc(100vh-10.5rem)] shrink-0 self-start bg-bg-subtle border-ie border-border overflow-y-auto" | |
| > |
🧰 Tools
🪛 GitHub Actions: chromatic
[error] 368-368: Storybook build failed (plugin vite:vue). RolldownError/SyntaxError: Duplicate attribute.
|
|
||
| <div class="flex-1 min-w-0 self-start"> | ||
| <div | ||
| class="sticky z-5 top-25 bg-bg border-b border-border px-4 py-2 flex items-center justify-between gap-2 text-nowrap overflow-x-auto max-w-full" | ||
| > | ||
| <div class="flex items-center gap-2"> | ||
| <!-- Sidebar toggle button --> | ||
| <button | ||
| type="button" | ||
| class="hidden md:flex items-center justify-center w-8 h-8 text-fg-subtle hover:text-fg transition-colors focus-visible:outline-accent/70 rounded" | ||
| :aria-label="$t(isSidebarCollapsed ? 'code.show_sidebar' : 'code.hide_sidebar')" | ||
| :aria-expanded="!isSidebarCollapsed" | ||
| aria-controls="file-tree-sidebar" | ||
| @click="toggleSidebar" | ||
| > | ||
| <span | ||
| class="w-4 h-4" | ||
| :class="isSidebarCollapsed ? 'i-lucide:sidebar-open' : 'i-lucide:sidebar-close'" | ||
| aria-hidden="true" | ||
| /> | ||
| </button> | ||
|
|
||
| <div | ||
| v-if="fileContent?.markdownHtml" | ||
| class="flex items-center gap-1 p-0.5 bg-bg-subtle border border-border-subtle rounded-md overflow-x-auto" | ||
| role="tablist" | ||
| aria-label="Markdown view mode selector" | ||
| > | ||
| <button | ||
| v-for="mode in markdownViewModes" | ||
| :key="mode.key" | ||
| role="tab" | ||
| class="px-2 py-1.5 font-mono text-xs rounded transition-colors duration-150 border border-solid focus-visible:outline-accent/70 inline-flex items-center gap-1.5" | ||
| :class=" | ||
| markdownViewMode === mode.key | ||
| ? 'bg-bg shadow text-fg border-border' | ||
| : 'text-fg-subtle hover:text-fg border-transparent' | ||
| " | ||
| :aria-selected="markdownViewMode === mode.key" | ||
| @click="markdownViewMode = mode.key" | ||
| > | ||
| <span class="inline-block h-3 w-3" :class="mode.icon" aria-hidden="true" /> | ||
| {{ mode.label }} | ||
| </button> | ||
| </div> | ||
| <!-- Breadcrumb navigation --> | ||
| <nav | ||
| :aria-label="$t('code.file_path')" | ||
| class="flex items-center gap-0.5 font-mono text-sm overflow-x-auto" | ||
| dir="ltr" | ||
| > | ||
| <NuxtLink | ||
| v-if="filePath" | ||
| :to="getCurrentCodeUrlWithPath()" | ||
| class="text-fg-muted hover:text-fg transition-colors shrink-0" | ||
| > | ||
| {{ $t('code.root') }} | ||
| </NuxtLink> | ||
| <span v-else class="text-fg shrink-0">{{ $t('code.root') }}</span> | ||
| <template v-for="(crumb, i) in breadcrumbs" :key="crumb.path"> | ||
| <span class="text-fg-subtle">/</span> | ||
| <NuxtLink | ||
| v-if="i < breadcrumbs.length - 1" | ||
| :to="getCurrentCodeUrlWithPath(crumb.path)" | ||
| class="text-fg-muted hover:text-fg transition-colors" | ||
| > | ||
| {{ crumb.name }} | ||
| </NuxtLink> | ||
| <span v-else class="text-fg">{{ crumb.name }}</span> | ||
| </template> | ||
| </nav> | ||
| </div> | ||
| <div class="flex items-center gap-2" v-if="isViewingFile && !isBinaryFile && fileContent"> | ||
| <button | ||
| v-if="selectedLines" | ||
| type="button" | ||
| class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors active:scale-95" | ||
| @click="copyPermalinkUrl" | ||
| > | ||
| {{ permalinkCopied ? $t('common.copied') : $t('code.copy_link') }} | ||
| </button> | ||
| <button | ||
| v-if="!!fileContent?.content" | ||
| type="button" | ||
| class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors inline-flex items-center gap-1 capitalize" | ||
| @click="copyFileContent()" | ||
| > | ||
| <span | ||
| class="w-3 h-3" | ||
| :class="fileContentCopied ? 'i-lucide:check' : 'i-lucide:file'" | ||
| /> | ||
| {{ fileContentCopied ? $t('common.copied') : $t('common.copy') }} | ||
| </button> | ||
| <a | ||
| :href="`https://cdn.jsdelivr.net/npm/${packageName}@${version}/${filePath}`" | ||
| target="_blank" | ||
| rel="noopener noreferrer" | ||
| class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors inline-flex items-center gap-1" | ||
| > | ||
| {{ $t('code.raw') }} | ||
| <span class="i-lucide:external-link w-3 h-3" /> | ||
| </a> | ||
| </div> | ||
| </div> | ||
| <div class="flex-1 grid grid-rows-[auto_1fr_auto] h-full min-h-full min-w-0 self-start"> |
There was a problem hiding this comment.
Template structure is broken — orphaned div and duplicate header components.
The new toolbar div starting at line 383 appears to be incomplete or improperly merged with the existing code. Observations:
- Line 383 opens
<div class="flex-1 min-w-0 self-start">but the closing</div>at line 485 ends the inner sticky toolbar, leaving the outer div unclosed. - Line 486 introduces another
<div class="flex-1 grid grid-rows-[auto_1fr_auto]...">which appears to be the original structure withCodeHeader. - This mismatch is causing the "Unexpected closing tag main" syntax error.
It looks like the new toolbar (lines 384-485) was intended to replace CodeHeader, but both remain in the template. You need to either:
- Remove the new inline toolbar and keep
CodeHeader, or - Remove
CodeHeaderand properly close the new structure.
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 473-473:
Property 'fileContentCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 471-471:
Property 'fileContentCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 467-467:
Property 'copyFileContent' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'. Did you mean 'fileContent'?
[failure] 461-461:
Property 'permalinkCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 459-459:
Property 'copyPermalinkUrl' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 444-444:
Property 'breadcrumbs' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 441-441:
Property 'breadcrumbs' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 411-411:
Property 'markdownViewModes' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'. Did you mean 'markdownViewMode'?
| v-for="mode in markdownViewModes" | ||
| :key="mode.key" | ||
| role="tab" | ||
| class="px-2 py-1.5 font-mono text-xs rounded transition-colors duration-150 border border-solid focus-visible:outline-accent/70 inline-flex items-center gap-1.5" | ||
| :class=" | ||
| markdownViewMode === mode.key | ||
| ? 'bg-bg shadow text-fg border-border' | ||
| : 'text-fg-subtle hover:text-fg border-transparent' | ||
| " | ||
| :aria-selected="markdownViewMode === mode.key" | ||
| @click="markdownViewMode = mode.key" | ||
| > | ||
| <span class="inline-block h-3 w-3" :class="mode.icon" aria-hidden="true" /> | ||
| {{ mode.label }} | ||
| </button> | ||
| </div> | ||
| <!-- Breadcrumb navigation --> | ||
| <nav | ||
| :aria-label="$t('code.file_path')" | ||
| class="flex items-center gap-0.5 font-mono text-sm overflow-x-auto" | ||
| dir="ltr" | ||
| > | ||
| <NuxtLink | ||
| v-if="filePath" | ||
| :to="getCurrentCodeUrlWithPath()" | ||
| class="text-fg-muted hover:text-fg transition-colors shrink-0" | ||
| > | ||
| {{ $t('code.root') }} | ||
| </NuxtLink> | ||
| <span v-else class="text-fg shrink-0">{{ $t('code.root') }}</span> | ||
| <template v-for="(crumb, i) in breadcrumbs" :key="crumb.path"> | ||
| <span class="text-fg-subtle">/</span> | ||
| <NuxtLink | ||
| v-if="i < breadcrumbs.length - 1" | ||
| :to="getCurrentCodeUrlWithPath(crumb.path)" | ||
| class="text-fg-muted hover:text-fg transition-colors" | ||
| > | ||
| {{ crumb.name }} | ||
| </NuxtLink> | ||
| <span v-else class="text-fg">{{ crumb.name }}</span> | ||
| </template> | ||
| </nav> | ||
| </div> | ||
| <div class="flex items-center gap-2" v-if="isViewingFile && !isBinaryFile && fileContent"> | ||
| <button | ||
| v-if="selectedLines" | ||
| type="button" | ||
| class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors active:scale-95" | ||
| @click="copyPermalinkUrl" | ||
| > | ||
| {{ permalinkCopied ? $t('common.copied') : $t('code.copy_link') }} | ||
| </button> | ||
| <button | ||
| v-if="!!fileContent?.content" | ||
| type="button" | ||
| class="px-2 py-1 font-mono text-xs text-fg-muted bg-bg-subtle border border-border rounded hover:text-fg hover:border-border-hover transition-colors inline-flex items-center gap-1 capitalize" | ||
| @click="copyFileContent()" | ||
| > | ||
| <span | ||
| class="w-3 h-3" | ||
| :class="fileContentCopied ? 'i-lucide:check' : 'i-lucide:file'" | ||
| /> | ||
| {{ fileContentCopied ? $t('common.copied') : $t('common.copy') }} |
There was a problem hiding this comment.
Missing function and variable definitions cause type check failures.
The template references several identifiers that are not defined in the <script setup> block:
| Line | Missing identifier |
|---|---|
| 411 | markdownViewModes |
| 441, 444 | breadcrumbs |
| 459 | copyPermalinkUrl |
| 461, 471, 473 | permalinkCopied, fileContentCopied |
| 467 | copyFileContent |
These are likely defined in the existing CodeHeader component that this new inline toolbar is duplicating. If you intend to keep the inline toolbar, you must define these in the script section. Otherwise, remove this duplicated toolbar and retain CodeHeader.
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 473-473:
Property 'fileContentCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 471-471:
Property 'fileContentCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 467-467:
Property 'copyFileContent' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'. Did you mean 'fileContent'?
[failure] 461-461:
Property 'permalinkCopied' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 459-459:
Property 'copyPermalinkUrl' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 444-444:
Property 'breadcrumbs' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 441-441:
Property 'breadcrumbs' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'.
[failure] 411-411:
Property 'markdownViewModes' does not exist on type '{ pkg: { isStale: boolean; _id: string; _rev?: string | undefined; name: string; description?: string | undefined; 'dist-tags': { latest?: string | undefined; } & Record<string, string>; ... 11 more ...; securityVersions?: PackageVersionInfo[] | undefined; } | undefined; ... 800 more ...; $npmApi: (url: string, o...'. Did you mean 'markdownViewMode'?
2 participants
🔗 Linked issue
Resolves #2058
🧭 Context
The package code browser previously had a fixed file tree sidebar that took up space even when not needed. This change adds a simple toggle to show/hide the sidebar, giving users better control over screen space on the code page
📚 Description
I implemented a sidebar toggle for the package code browser instead of full resizability. The toggle is simple, persistent, and matches the existing UI patterns.
Changes made:
Before :
After :