✨(frontend) restore skip to content link after header redesign#2510
✨(frontend) restore skip to content link after header redesign#2510Ovgodd wants to merge 1 commit into
Conversation
Ovgodd
force-pushed
the
feat/reimplement-skip-link
branch
from
74e8e2b to
484fe3a
Compare
Re-add skip link with responsive positioning and document title focus.
Ovgodd
force-pushed
the
feat/reimplement-skip-link
branch
from
484fe3a to
be4e6c5
Compare
WalkthroughThis change restores the skip-to-content link functionality after a header redesign. The SkipToContent component was updated to compute positioning based on router pathname, mobile/responsive state, and left-panel open state, with focus handling deferred until panel-close transitions complete on mobile. A new fallback for locating a doc title element as the focus target was added to layout utilities. The component is now mounted in the app root, a previously skipped e2e test was enabled, and the changelog was updated. Estimated code review effort: 2 (Simple) | ~12 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
Size Change: +575 B (+0.01%) Total Size: 4.37 MB 📦 View Changed
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@src/frontend/apps/impress/src/components/SkipToContent.tsx`:
- Around line 46-54: The leftBesideLogo calculation in SkipToContent should not
contain unexplained magic numbers; replace the 70px and 12px values with named
constants alongside HEADER_ICON_WIDTH and COLLAPSE_BUTTON_WIDTH. Update the
leftBesideLogo expression to reference those constants so the header spacing
logic stays maintainable and easy to adjust if the layout changes.
- Around line 37-41: The `handleClick` flow in `SkipToContent` schedules a
`window.setTimeout(focusContent, LEFT_PANEL_CLOSE_TRANSITION_MS)` without
cleanup, which can leave stale callbacks after unmount or repeated clicks. Store
the timeout ID in a ref inside `SkipToContent`, clear any pending timeout at the
start of `handleClick`, and also clear it in the component’s unmount cleanup so
`focusContent` only runs for the latest interaction.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: e223daf1-70e3-40d3-b8a8-4cc6efb247c8
📒 Files selected for processing (5)
CHANGELOG.mdsrc/frontend/apps/e2e/__tests__/app-impress/header.spec.tssrc/frontend/apps/impress/src/components/SkipToContent.tsxsrc/frontend/apps/impress/src/layouts/utils.tssrc/frontend/apps/impress/src/pages/_app.tsx
| if (isMobile && isPanelOpen) { | ||
| closePanel(); | ||
| window.setTimeout(focusContent, LEFT_PANEL_CLOSE_TRANSITION_MS); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Uncleaned setTimeout on unmount or repeated clicks.
When the mobile panel is open, window.setTimeout(focusContent, ...) is scheduled but never cleared. If the component unmounts (e.g., route change) or the user clicks again before 200 ms, stale or duplicate callbacks fire against the global DOM. Store the timeout ID in a ref and clear it on unmount and at the start of handleClick.
🛡️ Proposed fix
import { useRouter } from 'next/router';
-import { useState } from 'react';
+import { useEffect, useRef, useState } from 'react';
// ...
export const SkipToContent = () => {
// ...
const [isVisible, setIsVisible] = useState(false);
+ const timeoutRef = useRef<ReturnType<typeof setTimeout>>();
+
+ useEffect(() => {
+ return () => {
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
+ };
+ }, []);
const handleClick = (e: React.MouseEvent) => {
e.preventDefault();
+ if (timeoutRef.current) {
+ clearTimeout(timeoutRef.current);
+ }
const focusContent = () => {
// ...
};
if (isMobile && isPanelOpen) {
closePanel();
- window.setTimeout(focusContent, LEFT_PANEL_CLOSE_TRANSITION_MS);
+ timeoutRef.current = window.setTimeout(
+ focusContent,
+ LEFT_PANEL_CLOSE_TRANSITION_MS,
+ );
return;
}
focusContent();
};📝 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.
| if (isMobile && isPanelOpen) { | |
| closePanel(); | |
| window.setTimeout(focusContent, LEFT_PANEL_CLOSE_TRANSITION_MS); | |
| return; | |
| } | |
| import { useEffect, useRef, useState } from 'react'; | |
| // ... | |
| export const SkipToContent = () => { | |
| // ... | |
| const [isVisible, setIsVisible] = useState(false); | |
| const timeoutRef = useRef<ReturnType<typeof setTimeout>>(); | |
| useEffect(() => { | |
| return () => { | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| } | |
| }; | |
| }, []); | |
| const handleClick = (e: React.MouseEvent) => { | |
| e.preventDefault(); | |
| if (timeoutRef.current) { | |
| clearTimeout(timeoutRef.current); | |
| } | |
| const focusContent = () => { | |
| // ... | |
| }; | |
| if (isMobile && isPanelOpen) { | |
| closePanel(); | |
| timeoutRef.current = window.setTimeout( | |
| focusContent, | |
| LEFT_PANEL_CLOSE_TRANSITION_MS, | |
| ); | |
| return; | |
| } | |
| focusContent(); | |
| }; |
🤖 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 `@src/frontend/apps/impress/src/components/SkipToContent.tsx` around lines 37 -
41, The `handleClick` flow in `SkipToContent` schedules a
`window.setTimeout(focusContent, LEFT_PANEL_CLOSE_TRANSITION_MS)` without
cleanup, which can leave stale callbacks after unmount or repeated clicks. Store
the timeout ID in a ref inside `SkipToContent`, clear any pending timeout at the
start of `handleClick`, and also clear it in the component’s unmount cleanup so
`focusContent` only runs for the latest interaction.
| const leftBesideLogo = `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['4xs']} + 70px + 12px)`; | ||
| const isMobilePanelClosed = isMobile && !isPanelOpen; | ||
| const isDocEditorPage = pathname === '/docs/[id]'; | ||
|
|
||
| const left = isMobilePanelClosed | ||
| ? isDocEditorPage | ||
| ? `calc(${spacingsTokens['sm']} + ${COLLAPSE_BUTTON_WIDTH} + ${spacingsTokens['2xs']})` | ||
| : `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['2xs']})` | ||
| : leftBesideLogo; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Extract magic numbers in leftBesideLogo as named constants.
70px and 12px in the leftBesideLogo calc are unexplained. The other dimensions (HEADER_ICON_WIDTH, COLLAPSE_BUTTON_WIDTH, etc.) are named constants — these should follow the same pattern for maintainability and to prevent silent breakage when the header layout changes.
♻️ Proposed refactor
const HEADER_ROW_HEIGHT = '68px';
const HEADER_ICON_WIDTH = '32px';
const COLLAPSE_BUTTON_WIDTH = '48px';
+const HEADER_LOGO_TEXT_WIDTH = '70px';
+const HEADER_LOGO_GAP = '12px';
// ...
- const leftBesideLogo = `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['4xs']} + 70px + 12px)`;
+ const leftBesideLogo = `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['4xs']} + ${HEADER_LOGO_TEXT_WIDTH} + ${HEADER_LOGO_GAP})`;📝 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.
| const leftBesideLogo = `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['4xs']} + 70px + 12px)`; | |
| const isMobilePanelClosed = isMobile && !isPanelOpen; | |
| const isDocEditorPage = pathname === '/docs/[id]'; | |
| const left = isMobilePanelClosed | |
| ? isDocEditorPage | |
| ? `calc(${spacingsTokens['sm']} + ${COLLAPSE_BUTTON_WIDTH} + ${spacingsTokens['2xs']})` | |
| : `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['2xs']})` | |
| : leftBesideLogo; | |
| const HEADER_ROW_HEIGHT = '68px'; | |
| const HEADER_ICON_WIDTH = '32px'; | |
| const COLLAPSE_BUTTON_WIDTH = '48px'; | |
| const HEADER_LOGO_TEXT_WIDTH = '70px'; | |
| const HEADER_LOGO_GAP = '12px'; | |
| const leftBesideLogo = `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['4xs']} + ${HEADER_LOGO_TEXT_WIDTH} + ${HEADER_LOGO_GAP})`; | |
| const isMobilePanelClosed = isMobile && !isPanelOpen; | |
| const isDocEditorPage = pathname === '/docs/[id]'; | |
| const left = isMobilePanelClosed | |
| ? isDocEditorPage | |
| ? `calc(${spacingsTokens['sm']} + ${COLLAPSE_BUTTON_WIDTH} + ${spacingsTokens['2xs']})` | |
| : `calc(${spacingsTokens['sm']} + ${HEADER_ICON_WIDTH} + ${spacingsTokens['2xs']})` | |
| : leftBesideLogo; |
🤖 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 `@src/frontend/apps/impress/src/components/SkipToContent.tsx` around lines 46 -
54, The leftBesideLogo calculation in SkipToContent should not contain
unexplained magic numbers; replace the 70px and 12px values with named constants
alongside HEADER_ICON_WIDTH and COLLAPSE_BUTTON_WIDTH. Update the leftBesideLogo
expression to reference those constants so the header spacing logic stays
maintainable and easy to adjust if the layout changes.
| left: `calc(${spacingsTokens['base']} + 32px + ${spacingsTokens['3xs']} + 70px + 12px)`, | ||
| top, | ||
| left, | ||
| transform: 'translateY(-50%)', |
There was a problem hiding this comment.
I find the button quite big, I would have reduce the button size personnally:
| transform: 'translateY(-50%)', | |
| transform: 'translateY(-50%)', | |
| size="small" |
It will render that:
| - ✨(frontend) add a user menu #2463 | ||
| - ✨(frontend) new header and responsive harmonization #2471 | ||
| - ✨(backend) add management command to reset a Document #1882 | ||
| - ✨(frontend) restore skip to content link after header redesign #2510 |
There was a problem hiding this comment.
We just made a new release, this part should go under Unreleased.
2 participants
Purpose
Restore the skip-to-content link removed with the header redesign, so keyboard users can bypass navigation and reach main content.
Proposal
SkipToContentglobally in_app.tsx