New documentation website#3217
Conversation
| it('should render nothing when no issue is set', () => { | ||
| const count = el.renderRoot.querySelector('.count'); | ||
| expect(count).to.not.exist; | ||
| }); | ||
| }); | ||
|
|
||
| describe('with issue number', () => { | ||
| beforeEach(async () => { | ||
| mockSubIssues([{ state: 'open' }, { state: 'open' }, { state: 'closed' }]); | ||
| el = await fixture(html`<doc-open-issue-count .issue=${42}></doc-open-issue-count>`); | ||
| }); | ||
|
|
||
| it('should render the count of open sub-issues', () => { | ||
| const count = el.renderRoot.querySelector('.count'); | ||
| expect(count).to.exist; | ||
| expect(count).to.have.trimmed.text('2'); | ||
| }); |
There was a problem hiding this comment.
These tests query .count, but the component currently renders only text (no element with that class). Either update the component to wrap the count in an element (e.g., a span with a stable selector) or update the tests to assert on renderRoot.textContent instead.
| it('should render a link', () => { | ||
| const link = el.renderRoot.querySelector('a.leaf'); | ||
|
|
||
| expect(link).to.exist; | ||
| }); | ||
|
|
||
| it('should have the correct href', () => { | ||
| const link = el.renderRoot.querySelector('a.leaf'); | ||
|
|
||
| expect(link).to.have.attribute('href', '/'); | ||
| }); | ||
|
|
||
| it('should display the heading text', () => { | ||
| const link = el.renderRoot.querySelector('a.leaf'); | ||
|
|
||
| expect(link).to.have.trimmed.text('Home'); | ||
| }); |
There was a problem hiding this comment.
The tests query a.leaf, but the component renders the leaf link without that class (it uses part="leaf"). Update the selector in the tests or add the expected class in the component so the tests reflect the actual DOM structure.
| const getComponentMetadata = async path => { | ||
| const packageJsonPath = await findNearestPackageJson(join(repoRoot, path)); | ||
| if (!packageJsonPath) { | ||
| console.warn(`No package.json found for component at path "${path}".`); | ||
| return { status: null, version: null }; | ||
| } | ||
|
|
||
| try { | ||
| const packageJson = JSON.parse(await readFile(packageJsonPath, 'utf-8')), | ||
| repositoryUrl = packageJson.repository?.url, | ||
| repositoryDirectory = packageJson.repository?.directory; | ||
|
|
||
| return { | ||
| packageName: packageJson.name || null, | ||
| repositoryUrl, | ||
| repositoryDirectory, | ||
| status: packageJson.status || null, | ||
| version: packageJson.version || null | ||
| }; | ||
| } catch (err) { | ||
| console.error(`Error reading package.json for path "${path}":`, err); | ||
|
|
||
| return { status: null, version: null }; | ||
| } | ||
| }; | ||
|
|
||
| const sortByName = (a, b) => (a.name || '').localeCompare(b.name || ''); | ||
|
|
||
| const toTitle = name => { | ||
| const words = name.replace(/([A-Z])/g, ' $1').trim().toLowerCase().split(' '); | ||
| return words[0].charAt(0).toUpperCase() + words[0].slice(1) + (words.length > 1 ? ' ' + words.slice(1).join(' ') : ''); | ||
| }; | ||
|
|
||
| const escapeHtml = str => str.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | ||
| const backtickToCode = str => { | ||
| if (typeof str !== 'string') return str; | ||
| return escapeHtml(str).replace(/`([^`]+)`/g, '<code>$1</code>'); | ||
| }; | ||
|
|
||
| const transformDescriptions = item => { | ||
| if (!item || typeof item !== 'object') return item; | ||
| if (Array.isArray(item)) return item.map(transformDescriptions); | ||
| const result = { ...item }; | ||
| if ('description' in result) result.description = backtickToCode(result.description); | ||
| for (const key of Object.keys(result)) { | ||
| if (Array.isArray(result[key])) result[key] = result[key].map(transformDescriptions); | ||
| } | ||
| return result; | ||
| }; | ||
|
|
||
| export async function getComponents() { | ||
| const components = []; | ||
|
|
||
| for (const module of manifest.modules || []) { | ||
| const metadata = await getComponentMetadata(module.path), | ||
| name = metadata.packageName.split('/').at(-1); | ||
|
|
There was a problem hiding this comment.
getComponentMetadata() can return an object without packageName (e.g., when package.json is missing or fails to parse), but getComponents() unconditionally does metadata.packageName.split(...), which will crash the build. Ensure the metadata object always includes packageName (even if null) and guard the split/at logic with a fallback name.
| it('should have a heading role', () => { | ||
| expect(el).to.have.attribute('role', 'heading'); | ||
| }); | ||
|
|
||
| it('should have aria-level 2 by default', () => { | ||
| expect(el).to.have.attribute('aria-level', '2'); | ||
| }); |
There was a problem hiding this comment.
The spec asserts the host has role="heading" and aria-level, but the component renders a real <h2>/<h3> inside. Adding those ARIA attributes to the host would create duplicate heading semantics, so either update the tests to assert on the internal <h2>/<h3> only, or refactor the component to render a single heading semantic (host role or native heading element, not both).
| }); | ||
|
|
||
| it('should render a source area', () => { | ||
| expect(el.renderRoot.querySelector('.source')).to.exist; |
There was a problem hiding this comment.
This spec expects a .source element, but CodeExample renders the source inside <doc-code> without a .source wrapper. Either add a stable wrapper/class in the component or update the spec to query for doc-code / the slot[name="source"] instead.
| expect(el.renderRoot.querySelector('.source')).to.exist; | |
| expect(el.renderRoot.querySelector('doc-code')).to.exist; |
| this.addEventListener('keydown', this.#onKeydown); | ||
|
|
||
| navigation.addEventListener('navigate', this.#onNavigate); | ||
|
|
||
| // Set up roving tabindex after children are parsed | ||
| requestAnimationFrame(() => this.#initTabIndex()); | ||
| } | ||
|
|
||
| override disconnectedCallback(): void { | ||
| navigation.removeEventListener('navigate', this.#onNavigate); | ||
| this.removeEventListener('keydown', this.#onKeydown); |
There was a problem hiding this comment.
Using the global navigation object unguarded will throw in browsers that don’t implement the Navigation API. Guard the add/removeEventListener calls (e.g., if ('navigation' in window)), or feature-detect before referencing navigation at all.
|
|
||
| // declare global { | ||
| // interface HTMLElementTagNameMap { | ||
| // 'sl-accordion-item': AccordionItem; | ||
| // } | ||
| // } |
There was a problem hiding this comment.
There’s a commented-out HTMLElementTagNameMap declaration left at the end of the file. Commented code tends to rot and makes it unclear whether global typing is intended. Either remove it entirely, or reintroduce a proper (non-commented) global declaration in the appropriate place if these tag typings are still needed.
| /** | ||
| * Groups buttons together in a bar separated by whitespace. | ||
| * @customElement sl-button-bar | ||
| * @summary Groups buttons together in a bar separated by whitespace. | ||
| * | ||
| * ```html | ||
| * <sl-button-bar> | ||
| * <sl-button>Foo</sl-button> | ||
| * <sl-button>Bar</sl-button> | ||
| * </sl-button-bar> | ||
| * ``` | ||
| * @slot - Buttons to be grouped in the bar. | ||
| * | ||
| * @slot default - Buttons to be grouped in the bar. | ||
| * @cssState icon-only - Set when all buttons in the bar are icon-only. | ||
| * @cssState empty - Set when there are no buttons in the bar. | ||
| * @cssstate empty - Set when there are no buttons in the bar. | ||
| * @cssstate icon-only - Set when all buttons in the bar are icon-only. | ||
| */ |
There was a problem hiding this comment.
The JSDoc tags use @cssstate, but the rest of the codebase uses @cssState (e.g. packages/components/date-field/src/date-field.ts:39). If your CEM/doc tooling is case-sensitive, this may prevent these states from being documented. Rename the tags to @cssState for consistency.
| // Use dynamic import from dist to avoid CSS module resolution issues in browser tests | ||
| const { InstallInfo: InstallInfoClass } = await import('@sl-design-system/doc-components/install-info/install-info'); | ||
|
|
There was a problem hiding this comment.
This spec dynamically imports @sl-design-system/doc-components/.../install-info/install-info without the .js extension, but docs/components/package.json only exports paths ending in .js (e.g. ./install-info/install-info.js). With Node ESM + package exports this import will not resolve. Update the import to use the exported .js path.
| override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
| this.setAttribute('role', 'button'); | ||
| this.addEventListener('click', this.#onClick); | ||
| } |
There was a problem hiding this comment.
CopyButton sets role="button" on the host while also rendering a real <sl-button> (which itself renders a native <button>). Nesting interactive semantics like this can confuse assistive tech. Prefer letting the inner <sl-button> be the only interactive element (remove the host role/click handler and attach the click handler to the button).
| const { SiteNav, NavGroup, NavItem } = await import('@sl-design-system/doc-components/site-nav/site-nav'); | ||
|
|
There was a problem hiding this comment.
This spec dynamically imports @sl-design-system/doc-components/site-nav/site-nav without the .js extension, but the package exports only define .js subpaths. This will fail to resolve under Node ESM/package-exports. Import .../site-nav.js instead.
| const { OpenIssueCount: OpenIssueCountClass } = await import( | ||
| '@sl-design-system/doc-components/open-issue-count/open-issue-count' | ||
| ); |
There was a problem hiding this comment.
This spec dynamically imports @sl-design-system/doc-components/open-issue-count/open-issue-count without the .js extension, but the package exports only define .js subpaths. This will fail to resolve under Node ESM/package-exports. Import .../open-issue-count.js instead.
| </dd> | ||
| </dl> | ||
|
|
||
| <p class="summary">{{ component.summary }}</p> |
There was a problem hiding this comment.
component.summary is rendered without | safe, but the manifest processing converts backticks to <code> tags (and summaries may already contain HTML). This will likely be escaped by Nunjucks and render the tags as text. Consider rendering it as {{ component.summary | safe }} (and optionally only output the <p> when a summary exists).
| const { OpenIssueCount: OpenIssueCountClass } = await import( | ||
| '@sl-design-system/doc-components/open-issue-count/open-issue-count' | ||
| ); |
There was a problem hiding this comment.
The dynamic import path does not match the package exports map in docs/components/package.json, which only exports subpaths ending in .js (e.g. ./open-issue-count/open-issue-count.js). Importing @sl-design-system/doc-components/open-issue-count/open-issue-count will fail under Node/package-exports resolution; update the import to include the .js suffix.
| "repository": { | ||
| "type": "git", | ||
| "url": "https://github.com/sl-design-system/components.git", | ||
| "url": "https://github.com/sl-design-system/components", | ||
| "directory": "packages/components/tooltip" | ||
| }, |
There was a problem hiding this comment.
The repository.directory points to packages/components/tooltip, but this package is time-field. This metadata ends up incorrect in the generated docs/links. Update it to packages/components/time-field.
No description provided.