Skip to content

🐛 Fixed admin toolbar adding extra scroll to site pages#28674

Merged
JohnONolan merged 2 commits into
mainfrom
fix/admin-toolbar-body-scroll
Jun 18, 2026
Merged

🐛 Fixed admin toolbar adding extra scroll to site pages#28674
JohnONolan merged 2 commits into
mainfrom
fix/admin-toolbar-body-scroll

Conversation

@renatoworks

@renatoworks renatoworks commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Describe the problem you are solving

The admin toolbar shown to staff on the frontend is a position: fixed floating pill, but on mount it reserved padding-bottom on <body> equal to its height + 24px. That inflated the document's scroll height by roughly the bar's height, so every page gained a visible chunk of empty scroll whenever the toolbar was active, and it disrupted some theme layouts.

Why remove it rather than adjust it

The padding was almost certainly added to stop the fixed toolbar from overlapping page content at the bottom. But the trade-off is lopsided:

  • The toolbar is a small centered pill that only ever covers a narrow bottom-center sliver, yet the padding reserved a strip the full width of the page — far more space than the pill could occlude.
  • That cost was paid on every page load for anyone with the toolbar active (the phantom scroll + layout disruption), whereas the overlap it guarded against is minor and avoidable: the toolbar is staff-only and optional (only loads via ?admin=1), can be hidden, and has a minimized mode that shrinks it to a small nub.

Trading a guaranteed, universal scroll/layout issue for a rare, escapable cosmetic overlap is the better default, so the offset is removed entirely.

Changelog for devs

  • Removed apps/admin-toolbar/src/body-offset.js (the applyBodyOffset / clearBodyOffset helpers)
  • Removed the requestAnimationFrame body-offset call in src/index.js
  • Removed the now-unused BODY_PADDING_VAR constant from src/constants.js
  • Removed the obsolete body-padding assertion in test/admin-toolbar.test.js
  • Rebuilt the UMD bundle

Changelog for customers

  • Fixed the admin toolbar adding extra empty scroll space to the bottom of site pages

Notes

  • The toolbar's own positioning uses a literal bottom calc and never depended on the offset CSS var, so removing it does not move the toolbar.
  • clearBodyOffset was already dead code — it was exported but never imported anywhere, so deleting the whole module is safe.
  • Residual trade-off: when scrolled to the very bottom, page content at the centre can now sit under the floating pill instead of above it. Given the pill is small, optional, hideable, and minimisable (see above), this is an acceptable exchange for removing the scroll.
  • Lint passes; all 20 package tests pass.

Technical debt

  • None.

PR Scope (mark all that apply)

  • Improves the user experience
  • Enhances the readability of our code
  • Simplifies the maintenance of our software
  • Fixes a bug
  • Provide a core layer to allow one of the points above

Checklist before requesting a review (mark all that apply)

  • I have performed a self-review of my code
  • It's simple enough
  • The PR is not extensive
  • It includes documentation
  • I have added thorough tests.
  • Do we need to implement analytics?
  • Will this be part of a product update? If yes, please write one phrase about this update.

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: c4e50c4c-5b93-431e-a9af-c691277fb4a1

📥 Commits

Reviewing files that changed from the base of the PR and between 446e50d and 19f3cbf.

⛔ Files ignored due to path filters (1)
  • apps/admin-toolbar/umd/admin-toolbar.min.js is excluded by !**/*.min.js
📒 Files selected for processing (5)
  • apps/admin-toolbar/package.json
  • apps/admin-toolbar/src/body-offset.js
  • apps/admin-toolbar/src/constants.js
  • apps/admin-toolbar/src/index.js
  • apps/admin-toolbar/test/admin-toolbar.test.js
💤 Files with no reviewable changes (4)
  • apps/admin-toolbar/src/index.js
  • apps/admin-toolbar/test/admin-toolbar.test.js
  • apps/admin-toolbar/src/constants.js
  • apps/admin-toolbar/src/body-offset.js
🚧 Files skipped from review as they are similar to previous changes (1)
  • apps/admin-toolbar/package.json

Walkthrough

The PR removes the body offset feature from the admin toolbar. The BODY_PADDING_VAR constant is removed from constants.js. In index.js, the applyBodyOffset import and the requestAnimationFrame block that called it after rendering are removed; the post-render step now only sets frame.dataset.toolbarMounted = 'true'. The corresponding test assertion checking paddingBottom is removed and replaced with assertions for tooltip text, absence of the title attribute, and avatar image src. The package version is incremented to 0.1.1.

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: removing the admin toolbar's body offset mechanism that was causing unwanted scroll space on site pages.
Description check ✅ Passed The description is comprehensive and directly related to the changeset, explaining the problem, solution, rationale, and specific changes made to fix the issue.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/admin-toolbar-body-scroll

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud

nx-cloud Bot commented Jun 17, 2026

Copy link
Copy Markdown

🤖 Nx Cloud AI Fix

Ensure the fix-ci command is configured to always run in your CI pipeline to get automatic fixes in future runs. For more information, please see https://nx.dev/ci/features/self-healing-ci


View your CI Pipeline Execution ↗ for commit ac80d06

Command Status Duration Result
nx run @tryghost/admin:build ✅ Succeeded 14s View ↗
nx run-many -t lint -p @tryghost/admin-toolbar ✅ Succeeded 2s View ↗
nx run ghost:build:assets ✅ Succeeded 2s View ↗
nx run ghost:build:tsc ✅ Succeeded 6s View ↗
nx run-many --target=build --projects=@tryghost... ✅ Succeeded 1s View ↗

💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗


☁️ Nx Cloud last updated this comment at 2026-06-17 15:38:17 UTC

no ref

- the toolbar is a position:fixed floating pill, but it reserved padding-bottom on <body> equal to its height + 24px, which inflated the document's scroll height by the bar's height whenever it was active
- because the pill floats and never docks as a full-width bar, that reserved space served no purpose and only produced a visible phantom scroll
- removed the body offset entirely (body-offset.js, its rAF call in index.js, the now-unused BODY_PADDING_VAR constant, and the related test assertion)
no ref

- bumps the package version so CI's public-app version gate passes and the updated bundle is republished to the CDN
@renatoworks renatoworks force-pushed the fix/admin-toolbar-body-scroll branch from 446e50d to 19f3cbf Compare June 17, 2026 15:33
@renatoworks renatoworks requested a review from JohnONolan June 17, 2026 16:08
@JohnONolan JohnONolan merged commit 18046e5 into main Jun 18, 2026
41 checks passed
@JohnONolan JohnONolan deleted the fix/admin-toolbar-body-scroll branch June 18, 2026 05:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants