Fix wrong On-this-page entry on anchor jumps, and tables squeezed by a code block - #443
JakeSCahill wants to merge 2 commits into
Conversation
…ezed by a code block Two things the property reference pages surfaced. Both were pre-existing; the property links work just made in-page jumps common enough to notice. **The wrong TOC entry.** An anchor jump was offset twice: html carries scroll-padding-top, and the headings carried scroll-margin-top on top of it. The browser parks a linked heading at the sum, while the scroll-spy's activation line is scroll-padding-top alone, so the heading landed below the line and the pass picked the heading above it. Measured on a real page: parked at 180px, activation line at 104px, so clicking a link to rpc_server highlighted recovery_mode_enabled. Fixed at the root: one offset, on html, with the headings' 16/24px of breathing room folded into it. The browser and the scroll-spy now read the same value, and a linked heading parks level with the line (119px against 120px). Verified in a browser on the built pages: four targets, all correct after the change, all wrong before it. Also made hash navigation authoritative rather than inferred. onScroll cannot see an in-page click at all, because a hash change does not always move the scroll position enough to fire a scroll event, so hashchange only ever opened the collapsible group and never moved the highlight. It now activates from the hash, which also keeps this correct if the offsets ever drift apart again. Pulled the copy-pasted clear/add dance out of three call sites into setActive(). **Tables squeezed by a code block.** white-space: pre-wrap was already there, marked NEEDS REVIEW, and is not enough: it wraps between tokens but cannot break inside one, so a single long identifier still sets the cell's min-content width. `cloud_storage_inventory_hash_path_directory:` is 44 characters and did exactly that, taking the table to 542px inside a 354px phone and crushing the label column to 95px. overflow-wrap: anywhere is the keyword that lowers min-content, so the token breaks only when there is genuinely no room; break-word would not have helped. Measured with the real markup: 354px and no overflow, label column back to 118px. Two of the toc-collapsible tests asserted the old behaviour on purpose, with the message "the scroll pass picks the heading above". They now assert the target is active and the heading above it is not; their group assertions are unchanged.
✅ Deploy Preview for docs-ui ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change makes URL hash targets the active entries in the “On this page” TOC during page load and hash changes. It centralizes active-state handling and supports encoded fragments. Anchor offsets now use Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Low Sequence Diagram(s)sequenceDiagram
participant Browser
participant syncToHashTarget
participant setActive
participant scrollActiveIntoView
Browser->>syncToHashTarget: load or hashchange with URL hash
syncToHashTarget->>setActive: resolve and activate hash target
setActive->>setActive: clear previous active entry and reveal group
syncToHashTarget->>scrollActiveIntoView: scroll active entry into view
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The updated TOC synchronization and layout behavior has no remaining actionable risk identified in the reviewed change. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 69.23% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 13 functions across 3 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
validate-build.yml names each node --test suite explicitly, so a new one is invisible to CI until it is listed. tests/toc-active-anchor was passing locally and would never have run on a PR. head-meta, property-tooltips and negative-cache are in the same position and predate this; left alone here rather than widened in a fix for something else.
2 participants
Description
Two pre-existing bugs the property reference pages surfaced. The property-links work in redpanda-data/docs 2078 makes in-page jumps common enough that both became obvious.
Wrong "On this page" entry after an anchor jump. An anchor jump was offset twice:
htmlcarriesscroll-padding-topand the headings carriedscroll-margin-topon top of it. The browser parks a linked heading at the sum, while the scroll-spy's activation line isscroll-padding-topalone, so the heading landed below the line and the pass picked the heading above it. On a real page: parked at 180px, line at 104px, so a link torpc_serverhighlightedrecovery_mode_enabled.Now one offset, on
html, with the headings' breathing room folded in. Browser and scroll-spy read the same value. Hash navigation is also authoritative now, becauseonScrollcannot see an in-page click at all: a hash change does not always move the scroll position enough to fire a scroll event, sohashchangepreviously opened the collapsible group and never moved the highlight.Tables squeezed by a code block.
white-space: pre-wrapwas already there, markedNEEDS REVIEW, and is not enough: it wraps between tokens but cannot break inside one, so one long identifier sets the cell's min-content width.cloud_storage_inventory_hash_path_directory:is 44 characters and took the table to 542px inside a 354px phone, crushing the label column to 95px.overflow-wrap: anywherelowers min-content so the token breaks only when there is no room;break-worddoes not.Verification
Measured in a browser against the built pages, not reasoned from the CSS.
tests/toc-active-anchorsuite pins the behaviour at two landing positions, so it fails if the script goes back to inferring the active entry from pixels. All 17 JS suites pass.Two
toc-collapsibletests asserted the old behaviour deliberately ("the scroll pass picks the heading above"). They now assert the target is active; their group assertions are unchanged.