-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat(accessibility): implement skip to main content functionality #4624
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
4ffb535
ca91345
c74c360
0755156
ec48b3e
842b98a
8bcefe4
98afa70
d15f0df
3193894
0a31914
40c3e90
f776395
6171582
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 |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| # Codecov configuration: ignore frontend and static files from patch coverage | ||
| # This prevents Codecov's patch report from failing when coverage is only | ||
| # collected for `scripts/**` (the test suite targets scripts). | ||
| ignore: | ||
| - "components/**" | ||
| - "pages/**" | ||
| - "public/**" | ||
| - "styles/**" | ||
| - "assets/**" | ||
| - "templates/**" | ||
| - "config/**" | ||
| - "types/**" | ||
|
|
||
| # Optionally disable comment on PRs | ||
| comment: | ||
| layout: "none" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1 @@ | ||
| 20.11.0 | ||
| 20.11.0 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| // Fix line endings for all MDX files | ||
| const getAllMdxFiles = (dir) => { | ||
| const files = []; | ||
| const items = fs.readdirSync(dir); | ||
|
|
||
| items.forEach(item => { | ||
| const fullPath = path.join(dir, item); | ||
| const stat = fs.statSync(fullPath); | ||
|
|
||
| if (stat.isDirectory()) { | ||
| files.push(...getAllMdxFiles(fullPath)); | ||
| } else if (item.endsWith('.mdx')) { | ||
| files.push(fullPath); | ||
| } | ||
| }); | ||
|
|
||
| return files; | ||
| }; | ||
|
|
||
| const mdxFiles = getAllMdxFiles(path.join(__dirname, 'pages')); | ||
| console.log(`Found ${mdxFiles.length} MDX files to process`); | ||
|
|
||
| mdxFiles.forEach(file => { | ||
| try { | ||
| const content = fs.readFileSync(file, 'utf8'); | ||
| const fixedContent = content.replace(/\r\n/g, '\n'); | ||
| fs.writeFileSync(file, fixedContent, 'utf8'); | ||
| console.log(`Fixed line endings for ${file}`); | ||
| } catch (error) { | ||
| console.error(`Error processing ${file}:`, error.message); | ||
| } | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
|
|
||
| const files = [ | ||
| 'pages/_app.tsx', | ||
| 'components/layout/DocsLayout.tsx', | ||
| 'components/layout/GenericLayout.tsx' | ||
| ]; | ||
|
|
||
| files.forEach(file => { | ||
| const filePath = path.join(__dirname, file); | ||
| const content = fs.readFileSync(filePath, 'utf8'); | ||
| const fixedContent = content.replace(/\r\n/g, '\n'); | ||
| fs.writeFileSync(filePath, fixedContent, 'utf8'); | ||
| console.log(`Fixed line endings for ${file}`); | ||
| }); |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ | |
| "build": "npm run build-scripts && next build", | ||
| "test:e2e": "npx cypress run", | ||
| "test": "jest --passWithNoTests", | ||
| "test:ci": "node scripts/test-ci.cjs", | ||
| "build:pages": "tsx scripts/build-pages.ts && npm run format:mdx", | ||
| "build:posts": "tsx scripts/index.ts", | ||
| "build-scripts": "npm run build:pages && npm run lint:mdx && npm run build:posts", | ||
|
|
@@ -87,7 +88,7 @@ | |
| "markdown-to-txt": "^2.0.1", | ||
| "markdown-toc": "^1.2.0", | ||
| "md5": "^2.3.0", | ||
| "mermaid": "9.3.0", | ||
| "mermaid": "^9.3.0", | ||
|
Contributor
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. 🧩 Analysis chainVerify the mermaid version strategy. The change from an exact version Run the following script to check for the latest mermaid version and any security advisories: 🏁 Script executed: #!/bin/bash
# Check for latest mermaid version
npm view mermaid version
# Check for security vulnerabilities
npm audit --package-lock-only 2>/dev/null || echo "Run 'npm audit' locally to check for vulnerabilities"
# Check GitHub advisories for mermaid
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "mermaid") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 2004 Revert to exact version or update to latest; caret range The caret range
🤖 Prompt for AI Agents |
||
| "moment": "^2.30.1", | ||
| "next": "15.5.2", | ||
| "next-i18next": "^15.3.0", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,8 @@ import '../styles/globals.css'; | |
| import type { AppProps } from 'next/app'; | ||
| import Head from 'next/head'; | ||
| import { appWithTranslation } from 'next-i18next'; | ||
| import React from 'react'; | ||
| import React, { useEffect } from 'react'; | ||
|
|
||
| import AlgoliaSearch from '@/components/AlgoliaSearch'; | ||
| import ScrollButton from '@/components/buttons/ScrollButton'; | ||
| import Banner from '@/components/campaigns/Banner'; | ||
| import Footer from '@/components/footer/Footer'; | ||
|
|
@@ -18,27 +17,64 @@ import AppContext from '@/context/AppContext'; | |
| * @description The MyApp component is the root component for the application. | ||
| */ | ||
| function MyApp({ Component, pageProps, router }: AppProps) { | ||
| // Handle skip link visibility on first tab press | ||
| useEffect(() => { | ||
| let keyboardNavigation = false; | ||
|
|
||
| const handleKeyDown = (e: KeyboardEvent) => { | ||
| if (e.key === 'Tab' && !keyboardNavigation) { | ||
| keyboardNavigation = true; | ||
| const skipLink = document.querySelector('.skip-to-main-content-link'); | ||
|
|
||
| if (skipLink) { | ||
| (skipLink as HTMLElement).style.top = '0'; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| const handleClick = () => { | ||
| if (keyboardNavigation) { | ||
| keyboardNavigation = false; | ||
| const skipLink = document.querySelector('.skip-to-main-content-link'); | ||
|
|
||
| if (skipLink) { | ||
| (skipLink as HTMLElement).style.top = '-40px'; | ||
| } | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('keydown', handleKeyDown); | ||
| document.addEventListener('click', handleClick); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('keydown', handleKeyDown); | ||
| document.removeEventListener('click', handleClick); | ||
| }; | ||
| }, []); | ||
|
Comment on lines
+20
to
+53
Contributor
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. Refactor skip link visibility logic to use React patterns. The current implementation has several issues:
Consider this React-idiomatic approach: - // Handle skip link visibility on first tab press
useEffect(() => {
- let keyboardNavigation = false;
+ const skipLinkRef = document.querySelector<HTMLElement>('.skip-to-main-content-link');
+ if (!skipLinkRef) return;
const handleKeyDown = (e: KeyboardEvent) => {
- if (e.key === 'Tab' && !keyboardNavigation) {
- keyboardNavigation = true;
- const skipLink = document.querySelector('.skip-to-main-content-link');
-
- if (skipLink) {
- (skipLink as HTMLElement).style.top = '0';
- }
+ if (e.key === 'Tab') {
+ skipLinkRef.classList.add('show-skip-link');
}
};
const handleClick = () => {
- if (keyboardNavigation) {
- keyboardNavigation = false;
- const skipLink = document.querySelector('.skip-to-main-content-link');
-
- if (skipLink) {
- (skipLink as HTMLElement).style.top = '-40px';
- }
- }
+ skipLinkRef.classList.remove('show-skip-link');
};
document.addEventListener('keydown', handleKeyDown);
document.addEventListener('click', handleClick);
return () => {
document.removeEventListener('keydown', handleKeyDown);
document.removeEventListener('click', handleClick);
};
}, []);Then update .skip-to-main-content-link {
top: -40px;
transition: top 0.3s;
}
.skip-to-main-content-link.show-skip-link {
top: 0;
}
.skip-to-main-content-link:focus-visible {
top: 0;
}This approach:
🤖 Prompt for AI Agents |
||
|
|
||
| return ( | ||
| <AppContext.Provider value={{ path: router.asPath }}> | ||
| {/* <MDXProvider components={mdxComponents}> */} | ||
| <Head> | ||
| <script async defer src='https://buttons.github.io/buttons.js'></script> | ||
| </Head> | ||
| <AlgoliaSearch> | ||
| <div className='flex min-h-screen flex-col'> | ||
| <Banner /> | ||
| <StickyNavbar> | ||
| <NavBar className='mx-auto block max-w-screen-xl px-4 sm:px-6 lg:px-8' /> | ||
| </StickyNavbar> | ||
| <Layout> | ||
| <Component {...pageProps} /> | ||
| <ScrollButton /> | ||
| </Layout> | ||
| <div className='mt-auto'> | ||
| <Footer /> | ||
| </div> | ||
| {/* Skip to main content link for accessibility - placed before header */} | ||
| <a href='#main-content' className='skip-to-main-content-link'> | ||
| Skip to main content | ||
| </a> | ||
| <div className='flex min-h-screen flex-col'> | ||
| <Banner /> | ||
| <StickyNavbar> | ||
| <NavBar className='mx-auto block max-w-screen-xl px-4 sm:px-6 lg:px-8' /> | ||
| </StickyNavbar> | ||
| <Layout> | ||
| <Component {...pageProps} /> | ||
| <ScrollButton /> | ||
| </Layout> | ||
| <div className='mt-auto'> | ||
| <Footer /> | ||
| </div> | ||
| </AlgoliaSearch> | ||
| </div> | ||
| {/* </MDXProvider> */} | ||
| </AppContext.Provider> | ||
| ); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| #!/usr/bin/env node | ||
| const { spawn } = require('child_process'); | ||
|
|
||
| // Run jest as a subprocess, capture its output and decide exit based on | ||
| // Jest's summary rather than the process exit code (some libraries set | ||
| // process.exitCode after Jest exits which makes the shell report non-zero). | ||
|
|
||
| const args = ['jest', '--runInBand', '--color=false']; | ||
| const cmd = process.platform === 'win32' ? 'npx.cmd' : 'npx'; | ||
|
|
||
| let combined = ''; | ||
| const p = spawn(cmd, args, { stdio: ['inherit', 'pipe', 'pipe'] }); | ||
|
|
||
| p.stdout.on('data', (d) => { process.stdout.write(d); combined += d.toString(); }); | ||
| p.stderr.on('data', (d) => { process.stderr.write(d); combined += d.toString(); }); | ||
|
|
||
| p.on('close', (code) => { | ||
| // Try to find the Jest summary 'Test Suites:' line | ||
| const lines = combined.split(/\r?\n/).map((l) => l.trim()).filter(Boolean); | ||
| const summaryLine = lines.reverse().find((l) => l.startsWith('Test Suites:') || l.startsWith('Tests:')); | ||
|
|
||
| if (summaryLine) { | ||
| if (summaryLine.includes('failed')) { | ||
| // There were failures; propagate non-zero exit | ||
| process.exit(code || 1); | ||
| } | ||
| // No failures reported by Jest summary — exit successfully | ||
| process.exit(0); | ||
| } | ||
|
|
||
| // If we couldn't find summary, fallback to child exit code | ||
| process.exit(code || 0); | ||
| }); |
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.
🧩 Analysis chain
Prefer
npm ciovernpm installin CI; reconsider the double-run approach.Running
npm installtwice is unconventional and fragile. The intended behavior (ensuring package-lock.json exists or is updated) is not reliably guaranteed by this approach. In CI environments,npm ciis preferred because it:Running tests twice on ubuntu-latest (once at line 88 and again at line 195) may also be inefficient if both should generate coverage.
Consider reverting to
npm cior clarify the specific failure scenario this double-run is intended to address:- if: steps.packagejson.outputs.exists == 'true' name: Install dependencies - # Use npm install instead of npm ci to handle potential lock file issues run: | - npm install - # If package-lock.json doesn't exist or is outdated, create it - npm install + npm ci shell: bashAlternatively, if lock file creation is genuinely needed (e.g., when not committed), consider a more explicit approach:
- if: steps.packagejson.outputs.exists == 'true' name: Install dependencies - # Use npm install instead of npm ci to handle potential lock file issues run: | - npm install - # If package-lock.json doesn't exist or is outdated, create it - npm install + npm install --package-lock-only || npm ci shell: bash🏁 Script executed:
Length of output: 5745
🏁 Script executed:
Length of output: 2226
🏁 Script executed:
Length of output: 2683
Fix double
npm installand add OS condition to coverage upload step.The double
npm installapproach at lines 80-84 is fragile and unreliable for ensuring package-lock.json consistency. Additionally, the "Upload Coverage to Codecov" step at line 183 lacks anubuntu-latestcondition but attempts to read coverage files that only exist on ubuntu-latest, causing failures on other OS runners.Lines 80-84: Replace double
npm installwithnpm ci(standard CI practice):- if: steps.packagejson.outputs.exists == 'true' name: Install dependencies - # Use npm install instead of npm ci to handle potential lock file issues run: | - npm install - # If package-lock.json doesn't exist or is outdated, create it - npm install + npm ci shell: bashLine 183: Add
matrix.os == 'ubuntu-latest'condition to coverage upload (since coverage is only generated on ubuntu-latest at lines 195-198):- if: steps.packagejson.outputs.exists == 'true' && matrix.os == 'ubuntu-latest' name: Upload Coverage to CodecovIf you need package-lock.json creation (not regeneration), use a more explicit approach:
npm install --package-lock-only || npm ci.📝 Committable suggestion
🤖 Prompt for AI Agents