Align benchmarks page with site layout#275
Conversation
📝 WalkthroughWalkthroughRefactors the benchmarks page: DataTable component API simplified (removed Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 You can validate your CodeRabbit configuration file in your editor.If your editor has YAML language server, you can enable auto-completion and validation by adding |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/TunGo/src/pages/benchmarks.js (1)
184-208:⚠️ Potential issue | 🟠 MajorMake the horizontal scroll region keyboard reachable.
The new wrapper at Line 186 is now the only way to reach overflowing columns on narrow viewports, but a plain
<div>cannot receive focus. Keyboard-only users won't be able to scroll these tables horizontally unless the region is focusable.Suggested patch
function DataTable({header, rows, ariaLabel, className}) { return ( - <div className={Styles.tableWrap}> + <div + className={Styles.tableWrap} + role="region" + aria-label={ariaLabel} + tabIndex={0} + > <table className={`${Styles.dataTable}${className ? ` ${className}` : ''}`} aria-label={ariaLabel}> <thead>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TunGo/src/pages/benchmarks.js` around lines 184 - 208, The table's horizontal scroll wrapper (Styles.tableWrap) is not keyboard-focusable, preventing keyboard-only users from reaching overflowing columns; update the DataTable component so the wrapper div is focusable (add tabIndex={0}) and include an accessible label (role="region" with aria-label or aria-labelledby) so keyboard users can focus and scroll the container; ensure the focusable wrapper is applied where Styles.tableWrap is used inside the DataTable function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/TunGo/src/pages/benchmarks.module.css`:
- Around line 33-39: The .title rule (and other occurrences using a ch-based cap
like "max-width: 13ch") constrains translated headings to an English character
width and causes wrapping for longer locales; remove the ch-based max-width and
replace it with a locale-agnostic, responsive cap such as max-width: min(45rem,
100%) or simply max-inline-size: 100% so headings can expand naturally within
their container—update the .title rule and the other rules that use "max-width:
13ch" (the occurrences flagged around the other blocks) to use this responsive
alternative or remove the constraint entirely.
- Around line 2-12: Add a blank line separating the custom-property declarations
block from the normal declarations in the .page rule so the custom properties
(e.g., --bench-stack-gap, --bench-card-padding, --ifm-table-border-color, etc.)
are grouped and followed by an empty line before the first non-custom
declaration (background) to satisfy the stylelint declaration-empty-line-before
rule.
---
Outside diff comments:
In `@docs/TunGo/src/pages/benchmarks.js`:
- Around line 184-208: The table's horizontal scroll wrapper (Styles.tableWrap)
is not keyboard-focusable, preventing keyboard-only users from reaching
overflowing columns; update the DataTable component so the wrapper div is
focusable (add tabIndex={0}) and include an accessible label (role="region" with
aria-label or aria-labelledby) so keyboard users can focus and scroll the
container; ensure the focusable wrapper is applied where Styles.tableWrap is
used inside the DataTable function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61f9f681-a120-4071-9c08-3eac6ec9ab01
📒 Files selected for processing (2)
docs/TunGo/src/pages/benchmarks.jsdocs/TunGo/src/pages/benchmarks.module.css
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/TunGo/src/pages/benchmarks.module.css (2)
397-404: Minor: Table header font-size override appears redundant.Lines 398-400 set both
thandtdto0.82rem, but lines 402-404 immediately overridethto0.83rem. The 0.01rem difference (≈0.16px) is visually imperceptible. Consider consolidating these rules or removing the separatethoverride if the intent is to have uniform sizing.♻️ Suggested simplification
.dataTable th, .dataTable td { - font-size: 0.82rem; + font-size: 0.83rem; } - - .dataTable th { - font-size: 0.83rem; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TunGo/src/pages/benchmarks.module.css` around lines 397 - 404, The CSS contains a redundant override: .dataTable th, .dataTable td { font-size: 0.82rem; } followed immediately by .dataTable th { font-size: 0.83rem; }; remove the unnecessary rule or consolidate them so header and cell sizes are defined once—either delete the .dataTable th override and keep a single .dataTable th, .dataTable td rule (for uniform sizing) or merge into distinct, intentionally different sizes if you truly want headers larger; update the .dataTable th and .dataTable td selectors accordingly.
431-433: Optional: Redundant rule.Line 432 sets
.legendEntry { min-height: 0; }which is already declared in the base rule at line 314. Unless there's a specificity conflict (which doesn't appear to be the case here), this override can be removed.♻️ Suggested cleanup
`@media` screen and (max-width: 640px) { /* ... */ - .legendEntry { - min-height: 0; - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/TunGo/src/pages/benchmarks.module.css` around lines 431 - 433, Remove the redundant CSS declaration for .legendEntry { min-height: 0; } that duplicates the base rule; locate the duplicate .legendEntry rule and delete the min-height property (or the entire redundant block) so the stylesheet relies on the existing base rule for min-height, verifying there is no intended specificity override before removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/TunGo/src/pages/benchmarks.module.css`:
- Around line 397-404: The CSS contains a redundant override: .dataTable th,
.dataTable td { font-size: 0.82rem; } followed immediately by .dataTable th {
font-size: 0.83rem; }; remove the unnecessary rule or consolidate them so header
and cell sizes are defined once—either delete the .dataTable th override and
keep a single .dataTable th, .dataTable td rule (for uniform sizing) or merge
into distinct, intentionally different sizes if you truly want headers larger;
update the .dataTable th and .dataTable td selectors accordingly.
- Around line 431-433: Remove the redundant CSS declaration for .legendEntry {
min-height: 0; } that duplicates the base rule; locate the duplicate
.legendEntry rule and delete the min-height property (or the entire redundant
block) so the stylesheet relies on the existing base rule for min-height,
verifying there is no intended specificity override before removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aebcb180-989d-42c2-b367-84ebe64da19c
📒 Files selected for processing (2)
docs/TunGo/src/pages/benchmarks.jsdocs/TunGo/src/pages/benchmarks.module.css
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/TunGo/src/pages/benchmarks.js
1 participant
Summary
Verification
Summary by CodeRabbit
Refactor
Style
Accessibility