AdminPage: Give pages without a title or breadcrumbs a scroll surface under the admin page layout - #51872
AdminPage: Give pages without a title or breadcrumbs a scroll surface under the admin page layout#51872t-wright wants to merge 5 commits into
Conversation
…ge layout The jetpack-admin-page-layout mixin pins the content column to the viewport and expects a scrollable middle inside admin-ui's Page. An AdminPage with no title or breadcrumbs skips that Page, so its content had no scroll surface and the fixed column clipped everything below the fold. My Jetpack's #/connection screen hit this: the "Connect your user account" button sat out of reach on short viewports. Add a stable jp-admin-page__content class to the header-less content wrapper and have the mixin treat it as the scrollable middle. Tests lock both class hooks in place.
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Stats Data plugin: No scheduled milestone found for this plugin. If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Code Coverage SummaryCoverage changed in 4 files.
|
…itle Group the new content-hook selector with the existing scrollable-middle rule so the two cannot drift, and give the hook the same scrollbar-gutter as the title path in My Jetpack. Add a test that fails if the mixin stops selecting the hook, since jsdom cannot check layout. Backup's secondary-admin connection screen takes the same path and gets the same fix, so add its changelog entry. Rename "header-less" to "without a title or breadcrumbs", which is the real condition.
The fix reaches Backup through its jetpack-components and jetpack-base-styles dependencies. Consumers pick that up as a dependency update at release time, so a Backup entry breaks the convention.
The AdminPage test reads the layout mixin from disk, and the package type-checks its tests. Declare @types/node as a dev dependency and reference it from the test, as the charts package does.
coder-karen
left a comment
There was a problem hiding this comment.
I'm not familiar with these admin page components or styles nor the recent admin page changes, but can see this is a definite regression on the two mentioned connection screens. Thanks for catching it and for the fix! It tests well for me.
I do think there should be a changelog entry for every plugin that consumes My Jetpack, and separately Backup, since the changelog entry shoudn't just be for immediate packages touched but for the consumers who will be impacted.
I'm also wary of approving without the admin page context but also think this should be merged as soon as possible so ran an AI review to see if there's anything to flag. Here's what was flagged:
- The mixin doc block in admin-page-layout.scss doesn't yet mention the new
.jp-admin-page__content test/component.tsx(first test): this reads base-styles' SCSS fromnode_modulesand substring-matches the selector. Three problems: it passes on a match inside a comment, it drags @types/node + /// into a browser package for one assertion, and it guards only the base-styles copy — the duplicate inmy-jetpack/_inc/style.module.scsscan still drift. The three DOM tests carry the real value. Drop the file-read test, or move it to base-styles' own suite.
Drop the stylesheet guard test: it read base-styles' SCSS from node_modules, matched inside comments, and pulled Node types into a browser package for one assertion. The DOM tests carry the real value. The drift it guarded against is now structural: base-styles exposes the scrollable middle as a mixin, and both the layout and My Jetpack include it instead of copying the selector list. Compiled CSS is unchanged. Add changelog entries for the backup package and every plugin that surfaces the fixed screens, since users read plugin changelogs, not package ones. Document the content hook in the mixin's selector list.
There was a problem hiding this comment.
🟢 Approval recommended
The change is narrowly scoped, adds stable hooks plus targeted tests, and aligns consumers to a shared selector mixin without introducing risky behavioral changes elsewhere.
Pull request overview
This PR fixes a layout gap in the shared jetpack-admin-page-layout admin layout mixin where <AdminPage> screens rendered without a title/breadcrumbs had no internal scroll surface, making content (including the “Connect your user account” button) unreachable on short viewports.
Changes:
- Add a stable
.jp-admin-page__contenthook for<AdminPage>’s header-less (legacy) render path and extendjetpack-admin-page-layoutto treat it as the “scrollable middle”. - Introduce a reusable
jetpack-admin-page-scrollable-middleSCSS mixin so consumers can style the scroll surface without duplicating selectors (used by My Jetpack to applyscrollbar-gutter: stable). - Add unit tests to lock in the new class hook and selector expectations, plus changelog entries across affected projects/plugins.
File summaries
| File | Description |
|---|---|
| projects/js-packages/components/components/admin-page/index.tsx | Adds stable .jp-admin-page__content class to the legacy/header-less content wrapper. |
| projects/js-packages/components/components/admin-page/test/component.tsx | Adds tests asserting the .jp-admin-page__content hook behavior across header/title branches. |
| projects/js-packages/base-styles/admin-page-layout.scss | Adds jetpack-admin-page-scrollable-middle mixin and expands scrollable-middle targeting to include .jp-admin-page__content. |
| projects/packages/my-jetpack/_inc/style.module.scss | Switches to the new scrollable-middle mixin to apply scrollbar-gutter: stable consistently. |
| projects/js-packages/base-styles/changelog/fix-admin-page-header-less-scroll-surface | Changelog entry for the header-less scroll-surface fix in the layout mixin. |
| projects/js-packages/base-styles/changelog/add-admin-page-scrollable-middle-mixin | Changelog entry for the new consumer-facing scrollable-middle mixin. |
| projects/js-packages/components/changelog/fix-admin-page-header-less-scroll-surface | Changelog entry for the new stable AdminPage content hook. |
| projects/packages/my-jetpack/changelog/fix-admin-page-header-less-scroll-surface | Changelog entry for the My Jetpack connection screen scrolling fix. |
| projects/packages/backup/changelog/fix-admin-page-header-less-scroll-surface | Changelog entry for the secondary-admin connection screen scrolling fix. |
| projects/plugins/backup/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for the Backup secondary-admin connection screen scrolling fix. |
| projects/plugins/jetpack/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix (Jetpack type: bugfix). |
| projects/plugins/boost/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
| projects/plugins/protect/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
| projects/plugins/search/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
| projects/plugins/social/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
| projects/plugins/stats/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
| projects/plugins/videopress/changelog/fix-admin-page-header-less-scroll-surface | Plugin-level changelog entry for My Jetpack connection screen scrolling fix. |
Review details
- Files reviewed: 17/17 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Fixes MYJP-335
Proposed changes
My Jetpack's
#/connectionscreen doesn't scroll, so the "Connect your user account" button is unreachable on short viewports. Thejetpack-admin-page-layoutmixin pins the content column and only makes admin-ui'sPagemiddle scrollable. AnAdminPagewith no title or breadcrumbs skips thatPage, so its content had no scroll surface.AdminPage: add a stablejp-admin-page__contentclass to the content wrapper on that pathjetpack-admin-page-layout: treat that wrapper as the scrollable middle, grouped with the existing rulejetpack-admin-page-scrollable-middlemixin so consumers don't copy its selectorsscrollbar-gutteras the title path, through that mixinTwo pages take this path under the mixin: My Jetpack's connection screen and Backup's secondary-admin connection screen. Both now scroll, and their footer pins to the viewport bottom like every other page under the mixin.
Related product discussion/links
Does this pull request change what data or activity we track or use?
No.
Testing instructions
You need a site connected to Jetpack and an admin whose own WordPress.com account is not connected.
admin.php?page=my-jetpack#/connection.admin.php?page=jetpack-backup. The "Log in to continue" screen scrolls the same way.