chore(docs): add component preview to card list - #202
Open
coryrylan wants to merge 1 commit into
Open
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe documentation site now renders lazily loaded interactive component previews with sanitized templates, mocks, metadata, and responsive styling. Navigation consolidates Code, Monaco, and Markdown entries under Elements. Code, API, examples, descriptions, and preview configuration are updated. ChangesDocumentation previews and navigation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant IntersectionObserver
participant PreviewLoader
participant ComponentModule
IntersectionObserver->>PreviewLoader: observe preview card
PreviewLoader->>ComponentModule: import required module
PreviewLoader->>ComponentModule: initialize preview data
ComponentModule-->>PreviewLoader: component update
PreviewLoader-->>IntersectionObserver: mark preview ready or error
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/docs/elements/index.11ty.js`:
- Line 38: Update stripScripts to allow optional whitespace before the closing
angle bracket in </script> end tags, and repeatedly apply the replacement until
the template output no longer changes. Preserve the existing trimming behavior
and ensure residual or overlapping <script fragments are removed once the result
stabilizes.
- Around line 338-340: Update the generated script value in renderPreviewScript,
specifically the component tag and import entrypoint serialization around
component.imports.map, to escape the HTML-sensitive `</script` sequence after
JSON.stringify. Use a small shared helper for safe inline-script serialization
and apply it to both serialized values without changing the generated
module-loading behavior.
- Around line 431-443: Update the .component-preview[data-preview-ready]
.component-preview-content rule to reset the initial scale transform to full
size, and extend the .component-preview-content transition to include transform
alongside opacity.
- Around line 409-415: Add a .component-card-link:focus-visible rule that
applies a clear visible outline to the overlay link, ensuring keyboard focus
remains distinguishable while preserving the existing card-link positioning and
default non-focused appearance.
- Around line 54-57: Add a concise comment immediately above renderMonacoMock
documenting that tags containing “diff” return the two-pane Changes markup, tags
containing “problems” return the Problems list markup, and other tags return the
default pipeline.ts markup; keep the existing substring-based selection logic
unchanged.
- Around line 356-358: Guard the nve-sparkline lookup in the preview
initialization block with optional chaining before assigning its data, so a
missing element is skipped without throwing. Keep the existing sparkline data
and allow the remaining preview setup to continue normally.
- Line 324: Update the heading interpolation in the component rendering template
to pass component.title through the existing escapeHtml helper, matching the
escaping already used for the description and aria-label values.
- Around line 208-249: Add CSS rules in the <style> block for the
data-preview-size values produced by getPreviewSize, overriding
.component-preview’s fixed height so large previews (including nve-page and
nve-grid) are not clipped and wide previews receive their intended sizing. Keep
renderComponentCard’s data-preview-size attribute and the existing largePreviews
and widePreviews classifications intact.
- Around line 287-306: Collapse the chained maps in the component entry
construction into a single map. Within that map, call
getPreviewTemplate(component.tag) and construct each object once with
getPreviewImports(preview), preview, and getPreviewSize(component.tag), while
preserving the existing fields and final sortByTitle ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 9487593d-b539-4fd9-87dd-5d7f81d41efa
📒 Files selected for processing (2)
projects/core/src/gauge/gauge.examples.tsprojects/site/src/docs/elements/index.11ty.js
Comment on lines
+431
to
+443
| .component-preview-content { | ||
| display: grid; | ||
| place-items: center; | ||
| width: 100%; | ||
| height: 100%; | ||
| opacity: 0; | ||
| transform: scale(0.82); | ||
| transition: opacity var(--nve-ref-animation-duration-150); | ||
| } | ||
|
|
||
| .component-preview[data-preview-ready] .component-preview-content { | ||
| opacity: 1; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
The scale(0.82) transform is never reset, so every preview stays shrunk.
Line 437 applies transform: scale(0.82). Line 438 transitions opacity only. The ready rule at lines 441-443 changes opacity alone. No rule removes the transform, so each preview renders permanently at 82% scale instead of animating to full size. Reset the transform in the ready state and include it in the transition.
🐛 Proposed fix
opacity: 0;
transform: scale(0.82);
- transition: opacity var(--nve-ref-animation-duration-150);
+ transition:
+ opacity var(--nve-ref-animation-duration-150),
+ transform var(--nve-ref-animation-duration-150);
}
.component-preview[data-preview-ready] .component-preview-content {
opacity: 1;
+ transform: scale(1);
}📝 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.
Suggested change
| .component-preview-content { | |
| display: grid; | |
| place-items: center; | |
| width: 100%; | |
| height: 100%; | |
| opacity: 0; | |
| transform: scale(0.82); | |
| transition: opacity var(--nve-ref-animation-duration-150); | |
| } | |
| .component-preview[data-preview-ready] .component-preview-content { | |
| opacity: 1; | |
| } | |
| .component-preview-content { | |
| display: grid; | |
| place-items: center; | |
| width: 100%; | |
| height: 100%; | |
| opacity: 0; | |
| transform: scale(0.82); | |
| transition: | |
| opacity var(--nve-ref-animation-duration-150), | |
| transform var(--nve-ref-animation-duration-150); | |
| } | |
| .component-preview[data-preview-ready] .component-preview-content { | |
| opacity: 1; | |
| transform: scale(1); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/site/src/docs/elements/index.11ty.js` around lines 431 - 443, Update
the .component-preview[data-preview-ready] .component-preview-content rule to
reset the initial scale transform to full size, and extend the
.component-preview-content transition to include transform alongside opacity.
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
July 31, 2026 17:08
670501c to
d628c13
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 359-380: Bound the custom element definition wait in the
preview-loading flow by adding a timeout or equivalent settled fallback around
customElements.whenDefined(name), so an unregistered nve-* tag cannot block
Promise.all indefinitely. Ensure timeout failures are handled by the existing
error path and preview.dataset.previewReady is always set, including when
definition waiting fails or times out.
- Around line 36-57: Remove the redundant `xlink:href` entry from
`previewUrlAttributes`; `sanitizePreviewNode` should continue handling
namespaced xlink attributes through the existing `href` name check and
`isSafePreviewUrl` validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 55593e3b-d155-4e9b-9ee7-578fc3a54f37
📒 Files selected for processing (2)
projects/core/src/gauge/gauge.examples.tsprojects/site/src/docs/elements/index.11ty.js
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
July 31, 2026 21:27
d628c13 to
db101b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/core/src/drawer/drawer.ts`:
- Line 21: Update the JSDoc description for the Drawer symbol to use
grammatically correct singular wording and a direct description of its purpose,
preserving the existing examples of notifications, navigation, and settings.
In `@projects/core/src/notification/notification.ts`:
- Line 23: Update the JSDoc `@description` for the notification symbol to use
“real-time” as the modifier for “updates” and simplify the sentence according to
the repository’s technical writing style, while preserving the meaning that
notifications communicate important messages or status without interrupting the
user’s workflow.
In `@projects/core/src/tooltip/tooltip.ts`:
- Line 20: Update the JSDoc description for the tooltip component to state its
behavior directly with grammatically parallel wording, covering that it appears
on hover, focus, or tap and cannot contain interactive elements. Keep the change
limited to the description text and follow the repository’s technical writing
style.
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 387-407: Update the preview sizing around
.component-preview-content and its corresponding styles near the additional
occurrence so .preview-page and .preview-editor mocks fit entirely within their
.component-preview containers, including narrow single-column and two-column
spans. Increase the dedicated container height or apply sufficient scaling based
on each mock’s dimensions, while preserving the existing centered layout and
avoiding clipping.
- Around line 339-356: The IntersectionObserver lifecycle and fallback loading
need adjustment in the preview initialization flow. Update the observer callback
around observer.unobserve and loadPreview so it disconnects once all preview
targets have been handled, and change the fallback branch to schedule preview
loads in chunks rather than invoking loadPreview for every preview in one task;
preserve loading every preview when IntersectionObserver is unavailable.
- Around line 315-320: Bound the custom-element readiness wait in the preview
initialization flow around customElementNames and Promise.all so unregistered
nve-* elements cannot keep the preview pending indefinitely. Add a finite
timeout that rejects or otherwise releases the wait, while preserving immediate
completion for elements resolved by customElements.whenDefined(name).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 26e58148-e8c8-4338-b5d1-2a4129b4545f
📒 Files selected for processing (9)
projects/core/src/dialog/dialog.tsprojects/core/src/drawer/drawer.tsprojects/core/src/gauge/gauge.examples.tsprojects/core/src/notification/notification.tsprojects/core/src/toast/toast.tsprojects/core/src/tooltip/tooltip.tsprojects/markdown/src/markdown/markdown.examples.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/index.11ty.js
| /** | ||
| * @element nve-drawer | ||
| * @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings). Or use [Panel](./docs/elements/panel/) inline as its content couples with or closely relates to the content on the page (details, extra actions/options). [MDN Popover API](https://developer.mozilla.org/en-US/docs/Web/API/Popover_API) | ||
| * @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the drawer description grammar.
Drawer are is incorrect. Use a singular subject and a direct description.
As per coding guidelines, TypeScript JSDoc comments are linted by Vale and should follow the repository's technical writing style.
Proposed wording
- * `@description` Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings).
+ * `@description` A drawer displays content separate from the rest of the page, such as notifications, navigation, or settings.📝 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.
Suggested change
| * @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings). | |
| * `@description` A drawer displays content separate from the rest of the page, such as notifications, navigation, or settings. |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/core/src/drawer/drawer.ts` at line 21, Update the JSDoc description
for the Drawer symbol to use grammatically correct singular wording and a direct
description of its purpose, preserving the existing examples of notifications,
navigation, and settings.
Source: Coding guidelines
Comment on lines
+339
to
+356
| const previews = document.querySelectorAll('[data-component-preview]'); | ||
|
|
||
| if ('IntersectionObserver' in globalThis) { | ||
| const observer = new IntersectionObserver( | ||
| entries => { | ||
| entries.forEach(entry => { | ||
| if (!entry.isIntersecting) return; | ||
| observer.unobserve(entry.target); | ||
| void loadPreview(entry.target); | ||
| }); | ||
| }, | ||
| { rootMargin: '300px 0px' } | ||
| ); | ||
|
|
||
| previews.forEach(preview => observer.observe(preview)); | ||
| } else { | ||
| previews.forEach(preview => void loadPreview(preview)); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
The observer is never disconnected, and the fallback branch loads every preview at once.
Line 353 observes every card. Line 346 unobserves each entry after it intersects, so the observer stays alive with zero targets for the page lifetime. That is acceptable, but a disconnect() after the last target keeps the intent explicit.
The fallback at line 355 starts loadPreview for every card in the same task. Each call queries the whole subtree twice and awaits two animation frames. On a page with the full component catalog this creates one long task. Consider chunking the fallback, or accept it because IntersectionObserver is available in every supported browser.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/site/src/docs/elements/index.11ty.js` around lines 339 - 356, The
IntersectionObserver lifecycle and fallback loading need adjustment in the
preview initialization flow. Update the observer callback around
observer.unobserve and loadPreview so it disconnects once all preview targets
have been handled, and change the fallback branch to schedule preview loads in
chunks rather than invoking loadPreview for every preview in one task; preserve
loading every preview when IntersectionObserver is unavailable.
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
August 4, 2026 14:49
db101b8 to
8c2126b
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (4)
projects/site/src/docs/elements/index.11ty.js:76
flattenNestedContainerschecksattributes.includes('container='), which can produce false positives (e.g.,data-container=) and skip addingcontainer="flat"when the actualcontainerattribute is absent. Use an attribute-boundary regex instead so only a realcontainer=attribute prevents injection.
const flattenNestedContainers = template =>
template.replace(/<(nve-(?:grid|accordion-group|accordion))(?=[\s>])([^>]*)>/g, (match, tag, attributes) =>
attributes.includes('container=') ? match : `<${tag} container="flat"${attributes}>`
);
projects/core/src/tooltip/tooltip.ts:21
- The updated tooltip description sentence is ungrammatical (“Trigger tooltips by … and cannot …”), which reads like it’s missing a subject for the second clause. Rephrase to keep “tooltips” as the subject for both clauses.
* @element nve-tooltip
* @description A contextual popup that displays a plaintext description. Trigger tooltips by hovering, focusing, or tapping an element and cannot have interactive elements within them.
* @documentation https://nvidia.github.io/elements/docs/elements/tooltip/
projects/core/src/drawer/drawer.ts:22
- The drawer description uses incorrect subject/verb agreement (“Drawer are”). Since this line was edited, it’s a good opportunity to correct the grammar.
* @element nve-drawer
* @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings).
* @documentation https://nvidia.github.io/elements/docs/elements/drawer/
projects/core/src/notification/notification.ts:24
- “Real time” should be hyphenated as a compound adjective (“real-time updates”) in this description.
* @element nve-notification
* @description Displays real time updates without interrupting the user's workflow to communicate an important message or status.
* @documentation https://nvidia.github.io/elements/docs/elements/notification/
There was a problem hiding this comment.
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)
projects/site/src/docs/elements/index.11ty.js (1)
260-280: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild each catalog entry in one
.mappass.The first
.mapsetspreview: ''. The second.mapoverwrites it unconditionally and allocates a second object per component. The placeholder is never read.♻️ Proposed single-pass construction
.map(element => { const doc = docsByTag.get(element.name); return { description: element.manifest?.description?.trim() ?? `Documentation for ${doc.data.title}.`, href: doc.url, packageName: element.package, - preview: '', + preview: getPreviewTemplate(element.name), tag: element.name, title: doc.data.title, version: element.version }; }) - .map(component => { - const preview = getPreviewTemplate(component.tag); - - return { - ...component, - preview - }; - }) .sort(sortByTitle);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 260 - 280, Combine the two transformation stages into a single `.map` callback that sets `preview` directly to `getPreviewTemplate(element.name)` while constructing each catalog entry. Remove the placeholder `preview: ''` and the subsequent `.map(component => ...)`, preserving the existing fields and values.
♻️ Duplicate comments (2)
projects/site/src/docs/elements/index.11ty.js (2)
322-324: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard the
nve-sparklinelookup.
querySelectorreturnsnullwhen the preview markup contains nonve-sparkline.getPreviewTemplatefalls back to<code nve-text="code">nve-sparkline</code>when noDefaultexample exists for the tag. The assignment then throws, and thecatchblock marksdata-preview-errorinstead of finishing initialization. Use optional chaining.🛡️ Proposed fix
if (tag === 'nve-sparkline') { - preview.querySelector('nve-sparkline').data = [18, 22, 20, 24, 19, 28, 25, 30]; + const sparkline = preview.querySelector('nve-sparkline'); + + if (sparkline) sparkline.data = [18, 22, 20, 24, 19, 28, 25, 30]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 322 - 324, Guard the nve-sparkline element lookup in the tag initialization block by using optional chaining before assigning its data, so missing preview markup does not throw and trigger the error path.
315-320: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the
customElements.whenDefinedwait.
customElements.whenDefined(name)never settles until the name is registered. It does not reject and has no timeout. The docs page bundle does not register every preview tag, soPromise.allat line 320 can stay pending forever. The steps after it then never run: the sparkline seed at line 323 is skipped,updateCompleteis never awaited, andpreview.dataset.previewReadyat line 336 is never set. Thecatchblock never runs, so nothing is logged.Race the wait against a timeout.
🛡️ Proposed fix
+ const withTimeout = (promise, ms) => + Promise.race([promise, new Promise(resolve => setTimeout(resolve, ms))]); + const loadPreview = async preview => { const tag = preview.dataset.componentPreview; try { const customElementNames = new Set( [...preview.querySelectorAll('*')] .map(element => element.localName) .filter(name => name.startsWith('nve-')) ); - await Promise.all([...customElementNames].map(name => customElements.whenDefined(name))); + await withTimeout( + Promise.all([...customElementNames].map(name => customElements.whenDefined(name))), + 2000 + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 315 - 320, Bound the custom-element readiness wait in the preview initialization flow around customElementNames and Promise.all by racing each customElements.whenDefined(name) promise against a finite timeout, so unregistered preview tags cannot keep initialization pending forever. Preserve continuation to the subsequent seed, updateComplete, and previewReady steps when the timeout expires, while retaining the existing successful registration behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/_11ty/layouts/common.js`:
- Line 222: Add regression coverage for the navigation rendering logic around
the nve-tree-node entries: extend the existing tests to assert
/docs/code/codeblock/, /docs/markdown/, /docs/markdown/styles/, and each of the
five Monaco routes. Verify every route expands the Elements group where
applicable and marks the expected navigation entry as selected, while preserving
the existing /docs/elements/ and /docs/design-md/ assertions.
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 463-468: Adjust the `.component-preview` sizing or the
`.preview-page` and `.preview-editor` mock dimensions so both remain fully
visible after `.component-preview-content` applies its 0.85 scale within the
180px-high, overflow-hidden container. Preserve the existing grid layout while
ensuring neither mock is vertically clipped in the single- and two-column spans.
---
Outside diff comments:
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 260-280: Combine the two transformation stages into a single
`.map` callback that sets `preview` directly to
`getPreviewTemplate(element.name)` while constructing each catalog entry. Remove
the placeholder `preview: ''` and the subsequent `.map(component => ...)`,
preserving the existing fields and values.
---
Duplicate comments:
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 322-324: Guard the nve-sparkline element lookup in the tag
initialization block by using optional chaining before assigning its data, so
missing preview markup does not throw and trigger the error path.
- Around line 315-320: Bound the custom-element readiness wait in the preview
initialization flow around customElementNames and Promise.all by racing each
customElements.whenDefined(name) promise against a finite timeout, so
unregistered preview tags cannot keep initialization pending forever. Preserve
continuation to the subsequent seed, updateComplete, and previewReady steps when
the timeout expires, while retaining the existing successful registration
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 795ea63f-6008-49a4-96f6-29d8dbfdd1f9
📒 Files selected for processing (9)
projects/core/src/dialog/dialog.tsprojects/core/src/drawer/drawer.tsprojects/core/src/gauge/gauge.examples.tsprojects/core/src/notification/notification.tsprojects/core/src/toast/toast.tsprojects/core/src/tooltip/tooltip.tsprojects/markdown/src/markdown/markdown.examples.tsprojects/site/src/_11ty/layouts/common.jsprojects/site/src/docs/elements/index.11ty.js
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
August 4, 2026 21:02
8c2126b to
5291d34
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
github-actions
Bot
added
the
dependencies
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
August 4, 2026 21:03
5291d34 to
c188399
Compare
coryrylan
commented
Aug 4, 2026
| <nve-tree-node ${data.page.url.includes('/docs/monaco/diff-input/') ? 'highlighted selected' : ''}><a href="/docs/monaco/diff-input/">Monaco Diff Input</a></nve-tree-node> | ||
| <nve-tree-node ${data.page.url.includes('/docs/monaco/editor/') ? 'highlighted selected' : ''}><a href="/docs/monaco/editor/">Monaco Editor</a></nve-tree-node> | ||
| <nve-tree-node ${data.page.url.includes('/docs/monaco/diff-editor/') ? 'highlighted selected' : ''}><a href="/docs/monaco/diff-editor/">Monaco Diff Editor</a></nve-tree-node> | ||
| <nve-tree-node ${data.page.url.includes('/docs/monaco/problems/') ? 'highlighted selected' : ''}><a href="/docs/monaco/problems/">Monaco Problems</a></nve-tree-node> |
Collaborator
Author
There was a problem hiding this comment.
Flatten links in tree nav for easier discoverability
coryrylan
commented
Aug 4, 2026
| } | ||
|
|
||
| return highlighted + '\n'; | ||
| }; |
Collaborator
Author
There was a problem hiding this comment.
This is a workaround that prevents markdownit from wrapping nve-codeblock with additional pre/code tags. markdown-it/markdown-it#269
coryrylan
commented
Aug 4, 2026
| <nve-grid-cell>${i.description ?? ''}</nve-grid-cell> | ||
| i => /* html */ `<nve-grid-row role="row"> | ||
| <nve-grid-cell role="gridcell"><span nve-text="code nowrap">${escapeHtml(i.value)}</span></nve-grid-cell> | ||
| <nve-grid-cell role="gridcell">${i.description ?? ''}</nve-grid-cell> |
Collaborator
Author
There was a problem hiding this comment.
Grid handles this already but this helps agent search tools that don't run JS all the time
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
coryrylan
commented
Aug 4, 2026
| <p nve-text="body sm"><span class="preview-problems-marker">!</span> Unused environment variable</p> | ||
| <p nve-text="body sm muted">2 problems in 2 files</p> | ||
| </div> | ||
| </div>`; |
Collaborator
Author
There was a problem hiding this comment.
There are a handful of small mock dom UIs for the card previews. These are components that would be heavy payloads or to large to render (monaco, nve-page)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (9)
projects/site/src/_11ty/layouts/common.js:82
- The "hide non-ssr elements until defined" selector list dropped
nvd-canvas:not(:defined).nvd-canvasis still used for examples, so this can reintroduce a flash of unstyled/unupgraded canvases on docs pages.
nve-tree:not(:defined),
nve-grid:not(:defined),
nvd-canvas:not(:defined),
nve-button:not(:defined) {
visibility: hidden !important;
projects/site/package.json:171
PAGES_BASE_URLdefault changed to/, butIS_DEV_MODEinprojects/site/src/_11ty/layouts/common.jsstill treats/elements/preview/as the local-preview base path. With this default, local preview builds will no longer satisfy that condition, potentially changing dev/preview-only behavior unexpectedly.
"PAGES_BASE_URL": {
"external": true,
"default": "/"
},
projects/site/src/docs/elements/index.11ty.js:37
sanitizePreviewTemplate()filters out<script>and event-handler attributes, but it currently allows<style>elements. Because previews are inlined into the main docs page, a<style>tag inside an example could inject global CSS and affect the whole page.
const disallowedPreviewElements = new Set(['base', 'embed', 'iframe', 'link', 'meta', 'object', 'script']);
projects/site/src/docs/elements/index.11ty.js:57
sanitizePreviewTemplate()currently allows thestyleattribute through. Even without scripts, inline styles in preview HTML can leak outside the card (e.g., via selectors, positioning) and create hard-to-debug layout issues. Consider strippingstyleattributes for previews.
const name = attribute.name.toLowerCase();
if (name.startsWith('on') || name === 'srcdoc') return false;
if (previewUrlAttributes.has(name)) return isSafePreviewUrl(attribute.value);
projects/core/src/tooltip/tooltip.ts:20
- The updated tooltip description is ungrammatical (it loses the subject for the second clause). It reads as if "Trigger tooltips ... and cannot ...".
* @description A contextual popup that displays a plaintext description. Trigger tooltips by hovering, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/toast/toast.ts:24
- The toast description says toasts "cannot have interactive elements" but this component can render an interactive close button (
closablerenders an<nve-icon-button>). The description should match the actual behavior.
* @description A contextual popup that displays a status. Toasts are [triggered](https://w3c.github.io/aria/#tooltip) by clicking, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/notification/notification.ts:23
- Minor copy edit: "real time" should be hyphenated when used as a compound adjective ("real-time updates").
* @description Displays real time updates without interrupting the user's workflow to communicate an important message or status.
projects/core/src/drawer/drawer.ts:21
- Grammar: this should be plural ("Drawers are...") or use a singular construction ("A drawer is...").
* @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings).
projects/core/src/dialog/dialog.ts:24
- Grammar: "Dialog is a component" reads awkwardly; use a singular article and add "the" before "main content" for clarity.
* @description Dialog is a component that appears above main content. A modal dialog displays critical information that requires user attention and interrupts user flow.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
projects/site/src/docs/elements/index.11ty.js (3)
463-468: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
.preview-pageis clipped by the 180px preview container.
.component-previewsetsheight: 180pxandoverflow: hiddenat lines 391-392..component-preview-contentscales by 0.85 at line 406..preview-pageis 560×260, so it still needs about 476×221 after scaling. Thenve-pagecard shows a cropped mock. Scale this mock down, or give it a taller container.🎨 Proposed fix
.preview-page { width: 560px; height: 260px; overflow: hidden; background: var(--nve-sys-layer-canvas-background); + transform: scale(0.62); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 463 - 468, Update the preview sizing around .component-preview, .component-preview-content, and .preview-page so the scaled 560×260 mock fits within the 180px-tall overflow-hidden container without cropping; prefer reducing the mock’s effective dimensions or scale while preserving the existing preview layout.
322-324: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard the
nve-sparklinelookup against a missing element.
querySelectorreturnsnullwhen the preview markup contains nonve-sparkline. That happens whendefaultExamplesByTaghas noDefaultexample for the tag, becausegetPreviewTemplatethen falls back to<code nve-text="code">nve-sparkline</code>. The property assignment then throws and the card is marked withdata-preview-error. Guard the lookup.🛡️ Proposed fix
if (tag === 'nve-sparkline') { - preview.querySelector('nve-sparkline').data = [18, 22, 20, 24, 19, 28, 25, 30]; + const sparkline = preview.querySelector('nve-sparkline'); + + if (sparkline) sparkline.data = [18, 22, 20, 24, 19, 28, 25, 30]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 322 - 324, Guard the nve-sparkline assignment in the preview-generation logic by checking that preview.querySelector('nve-sparkline') returns an element before setting its data property. Preserve the existing data assignment when the element is present and avoid throwing when the fallback template omits it.
315-320: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBound the
customElements.whenDefinedwait.
customElements.whenDefined(name)never settles until the name is registered. It does not reject and has no timeout. This file no longer emits component imports, so registration depends on the page bundle. If a preview contains annve-*tag that the bundle does not register,Promise.all(...)stays pending, thecatchnever runs, andpreview.dataset.previewReadyat line 336 is never set. Add a finite timeout so the wait always settles.🛡️ Proposed fix
+ const withTimeout = (promise, ms) => + Promise.race([promise, new Promise(resolve => setTimeout(resolve, ms))]); + const loadPreview = async preview => { const tag = preview.dataset.componentPreview; try { const customElementNames = new Set( [...preview.querySelectorAll('*')] .map(element => element.localName) .filter(name => name.startsWith('nve-')) ); - await Promise.all([...customElementNames].map(name => customElements.whenDefined(name))); + await withTimeout( + Promise.all([...customElementNames].map(name => customElements.whenDefined(name))), + 2000 + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@projects/site/src/docs/elements/index.11ty.js` around lines 315 - 320, Bound the custom-element readiness wait in the preview setup around customElementNames and customElements.whenDefined so each unresolved element settles after a finite timeout. Preserve immediate completion for registered names and ensure timeout handling allows the surrounding flow to continue to preview.dataset.previewReady rather than leaving Promise.all pending.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@projects/site/src/_11ty/layouts/docs.css`:
- Around line 219-221: In the nve-codeblock CSS rule, add a blank line between
the --padding declaration and the anchor-name declaration to satisfy the
declaration-empty-line-before Stylelint rule.
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 267-280: Collapse the chained maps that build the component
entries into a single map callback. In the map producing each component from
element and doc data, compute getPreviewTemplate(element.name) directly for
preview and return the complete object once, removing the unused preview
placeholder and second object allocation.
---
Duplicate comments:
In `@projects/site/src/docs/elements/index.11ty.js`:
- Around line 463-468: Update the preview sizing around .component-preview,
.component-preview-content, and .preview-page so the scaled 560×260 mock fits
within the 180px-tall overflow-hidden container without cropping; prefer
reducing the mock’s effective dimensions or scale while preserving the existing
preview layout.
- Around line 322-324: Guard the nve-sparkline assignment in the
preview-generation logic by checking that preview.querySelector('nve-sparkline')
returns an element before setting its data property. Preserve the existing data
assignment when the element is present and avoid throwing when the fallback
template omits it.
- Around line 315-320: Bound the custom-element readiness wait in the preview
setup around customElementNames and customElements.whenDefined so each
unresolved element settles after a finite timeout. Preserve immediate completion
for registered names and ensure timeout handling allows the surrounding flow to
continue to preview.dataset.previewReady rather than leaving Promise.all
pending.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 7028b96e-a796-447a-9c16-2a82e0bb864e
📒 Files selected for processing (14)
projects/core/src/dialog/dialog.tsprojects/core/src/drawer/drawer.tsprojects/core/src/gauge/gauge.examples.tsprojects/core/src/notification/notification.tsprojects/core/src/toast/toast.tsprojects/core/src/tooltip/tooltip.tsprojects/markdown/src/markdown/markdown.examples.tsprojects/site/package.jsonprojects/site/src/_11ty/layouts/common.jsprojects/site/src/_11ty/layouts/docs.cssprojects/site/src/_11ty/libraries/markdown.jsprojects/site/src/_11ty/shortcodes/api.jsprojects/site/src/_11ty/shortcodes/example.jsprojects/site/src/docs/elements/index.11ty.js
Comment on lines
+219
to
+221
| nve-codeblock { | ||
| --padding: var(--nve-ref-space-lg); | ||
| anchor-name: --codeblock; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required blank line before anchor-name.
Stylelint 17.14.0 reports declaration-empty-line-before at Line 221. Add an empty line between --padding and anchor-name.
Proposed fix
nve-codeblock {
--padding: var(--nve-ref-space-lg);
+
anchor-name: --codeblock;📝 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.
Suggested change
| nve-codeblock { | |
| --padding: var(--nve-ref-space-lg); | |
| anchor-name: --codeblock; | |
| nve-codeblock { | |
| --padding: var(--nve-ref-space-lg); | |
| anchor-name: --codeblock; |
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 221-221: Expected empty line before declaration (declaration-empty-line-before)
(declaration-empty-line-before)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@projects/site/src/_11ty/layouts/docs.css` around lines 219 - 221, In the
nve-codeblock CSS rule, add a blank line between the --padding declaration and
the anchor-name declaration to satisfy the declaration-empty-line-before
Stylelint rule.
Source: Linters/SAST tools
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (8)
projects/site/src/docs/elements/index.11ty.js:37
- The preview sanitizer blocks <script>, <iframe>, etc., but still allows <style> tags. Because <style> is not scoped, a component example could inject CSS that affects the entire docs page (and potentially be used for UI redress attacks). Add
styleto the disallowed element set so preview HTML cannot introduce global styles.
const disallowedPreviewElements = new Set(['base', 'embed', 'iframe', 'link', 'meta', 'object', 'script']);
projects/site/src/docs/elements/index.11ty.js:321
customElements.whenDefined()never rejects; if any nestednve-*element in the preview is not registered, thisawait Promise.all(...)will hang forever and the card will never reach the "ready" state. Consider adding a timeout (and surfacing the error state) so previews fail closed instead of hanging indefinitely.
await Promise.all([...customElementNames].map(name => customElements.whenDefined(name)));
projects/site/src/docs/elements/index.11ty.js:324
- This assumes the preview always contains an
<nve-sparkline>element. If the default example is missing/renamed (or sanitization removes the element), this will throw and mark the preview as errored unnecessarily. Guard against a missing element before assigning.data.
if (tag === 'nve-sparkline') {
preview.querySelector('nve-sparkline').data = [18, 22, 20, 24, 19, 28, 25, 30];
}
projects/site/package.json:171
- Changing the default
PAGES_BASE_URLto/may break dev/preview detection logic that still special-cases/elements/preview/(seeprojects/site/src/_11ty/layouts/common.jswhereIS_DEV_MODEchecksprocess.env.PAGES_BASE_URL === '/elements/preview/'). Either update that logic to match the new default /LOCAL_PREVIEW, or keep the prior default here to avoid silently changing behavior.
"PAGES_BASE_URL": {
"external": true,
"default": "/"
},
projects/core/src/drawer/drawer.ts:21
- Grammar: this should be plural ("Drawers are...") or otherwise rephrased. As written, "Drawer are" reads as a typo in the public JSDoc description.
* @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings).
projects/core/src/tooltip/tooltip.ts:20
- This sentence is ungrammatical as written ("Trigger tooltips ... and cannot..."). Consider rephrasing so the subject is consistent (e.g., "Tooltips are triggered..." / "They cannot...").
* @description A contextual popup that displays a plaintext description. Trigger tooltips by hovering, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/toast/toast.ts:24
- This sentence is ungrammatical as written ("...by clicking... and cannot have..."). Consider rephrasing so the subject is consistent and reads cleanly in generated docs.
* @description A contextual popup that displays a status. Toasts are [triggered](https://w3c.github.io/aria/#tooltip) by clicking, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/notification/notification.ts:23
- In prose, "real-time" is typically hyphenated when used as a compound modifier. This is a public JSDoc description, so it’s worth tightening the wording.
* @description Displays real time updates without interrupting the user's workflow to communicate an important message or status.
- add small inline preview of the component to the card - large components like monaco have small mock html/css inlined visuals Signed-off-by: Cory Rylan <crylan@nvidia.com>
coryrylan
force-pushed
the
topic-docs-improvements
branch
from
August 5, 2026 16:00
c188399 to
bc5afdf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated no new comments.
Suppressed comments (5)
projects/core/src/tooltip/tooltip.ts:20
- The description sentence is ungrammatical ("Trigger tooltips by… and cannot…" mixes imperative + clause). Rephrase as two declarative clauses so it reads correctly in generated docs.
* @description A contextual popup that displays a plaintext description. Trigger tooltips by hovering, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/toast/toast.ts:24
- The description links to the ARIA tooltip definition for "triggered", which doesn’t match a toast (this component sets role="alert"). Consider removing the tooltip link and using plain language to avoid incorrect cross-references in docs.
* @description A contextual popup that displays a status. Toasts are [triggered](https://w3c.github.io/aria/#tooltip) by clicking, focusing, or tapping an element and cannot have interactive elements within them.
projects/core/src/notification/notification.ts:23
- "real time" should be hyphenated as "real-time" when used as a compound modifier.
* @description Displays real time updates without interrupting the user's workflow to communicate an important message or status.
projects/core/src/drawer/drawer.ts:21
- The description has a subject/verb agreement issue ("Drawer are"). Adjust to singular or plural so the generated docs read correctly.
* @description Drawer are to display content that is out of context of the rest of the page (notifications, navigation, settings).
projects/core/src/dialog/dialog.ts:24
- Minor grammar: "Dialog is" reads awkwardly, and "interrupts user flow" is missing an article. Rephrase so the generated docs read cleanly.
* @description Dialog is a component that appears above main content. A modal dialog displays critical information that requires user attention and interrupts user flow.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Updates