fix(a11y): move readme after sidebar in DOM order#1072
fix(a11y): move readme after sidebar in DOM order#1072alexdln merged 3 commits intonpmx-dev:mainfrom
Conversation
This is to solve a level A WCAG failure (SC 2.4.3).
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe changes reposition the sidebar and README section in the package page layout. In the Skeleton component, the README area is moved from between Vulns and Sidebar to after the Sidebar. The responsive grid layout is adjusted at tablet sizes so README is positioned below the vulns and sidebar blocks. In the page component, the PackageSidebar block is moved from after the vulnerabilities section to before the README section, with corresponding CSS grid updates at the 1024px breakpoint to maintain alignment with the new sidebar placement. Possibly related PRs
Suggested labels
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)
📝 Coding Plan
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 Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
I agree that the order needs to be changed (discord), but I disagree about the reason. If the problem is only with A11Y, we need to change the order of the elements in the DOM (make <Sidebar> higher than <Readme>). As for the visual reordering, I think it's better to decide what experience we want to achieve (maybe for example collapsing sidebar sections on mobile devices is better) |
|
In #1072 (comment), @alexdln said:
If we did that it would cause the same WCAG failure for the “tablet” and desktop layouts because they would no longer match their grid order. Moving DOM specifically when the mobile breakpoint matches (i.e. using It’s that complexity and “whack-a-mole” problem that I’m trying to address with this PR. To make matters simple: DOM order must match the visual order for every breakpoint (without the help of JavaScript). I’m doing so at the cost of design, but it was a design that should never have been tenable because it did not consider accessibility. With all of that said, if we want to revisit the visual order that’s totally fine. The purpose of this PR less so to make a design change and more so to remediate the accessibility issue in a way that is most performant and universal (i.e. it works with whatever no-JS experience there is). |
It shouldn't. Navigation, toc, and priority elements to the right and left of main content are fairly standard practice. The current complaint was that focus jumping abruptly to the bottom of the page, and that shouldn't happen. With the option I'm talking about, focus should move correctly, since our right panel is sticky |
|
In #1072 (comment), @alexdln said:
That’s a really fair point.
With your option applied, I think the desktop view is fine. The focus jump remains in view. In the “tablet” view (I use a 14" laptop so I get this view if I open the vertical-tab-sidebar in my browser, which is pretty much always), it is a bit strange when the focus initially moves to the right column instead of the left column when traversing through the page. At this breakpoint, the “Getting started” + “vulnerabilities” sections are both comically wide when they don’t need to be. If we make both of them only take up one column and bump the sidebar up, that improves the experience to be more like desktop (and makes it look nicer IMO). Proposed grid layout change for tablet view
@media (min-width: 1024px) {
.package-page {
grid-template-columns: 2fr 1fr;
grid-template-areas:
'header header'
'details details'
- 'install install'
+ 'install sidebar'
- 'vulns vulns'
+ 'vulns sidebar'
'readme sidebar';
grid-template-rows: auto auto auto auto 1fr;
}
}What do you think?
I agree and, yes, I think collapsing the sidebar sections on mobile would be a good fix. |
|
lgtm about tablet (source)
|
|
(marking as draft while you work on it) |
Also, moves the sidebar placement up within its column for tablet.

Resolves #1069 which reports a WCAG 2.2 SC 2.4.3 Focus Order (Level A) failure.
This PR moves the README after the sidebar in the DOM order, so that it matches on mobile. On tablet and desktop, this changes the tab order, so the focus will jump to the sidebar which we determined was okay, since going from one column to the next and then back is not uncalled for (e.g. it’s often the case with table of contents). The grid layout was slightly tweaked for the tablet view which just moves the sidebar up a bit so that the focus jump makes more sense.