Settings: migrate to shared AdminPage component#47490
Conversation
vianasw
added
[Status] Needs Review
github-actions
bot
added
[Plugin] Jetpack
|
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. |
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
There was a problem hiding this comment.
Pull request overview
Migrates the Jetpack Settings wp-admin experience to the shared @automattic/jetpack-components AdminPage layout, replacing the legacy Masthead/Footer and Calypso-based Settings navigation with a new React Router tab implementation.
Changes:
- Swaps Settings routes to render inside a new
SettingsAdminPagewrapper (shared header/footer viaAdminPage). - Introduces
SettingsNavTabsusingNavLinkand adds associated styles. - Adds a Settings-specific CSS override to neutralize
AdminPage’smargin-leftoffset given Jetpack’s custom#wpcontentpadding.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/plugins/jetpack/changelog/update-settings-use-adminpage | Changelogger entry for the Settings UI migration. |
| projects/plugins/jetpack/_inc/client/scss/style.scss | Loads styles for the new Settings tabs component. |
| projects/plugins/jetpack/_inc/client/main.jsx | Routes Settings paths through the new SettingsAdminPage wrapper. |
| projects/plugins/jetpack/_inc/client/components/settings-nav-tabs/index.jsx | New Settings navigation tabs + search integration. |
| projects/plugins/jetpack/_inc/client/components/settings-nav-tabs/style.scss | Styling for the new tabs + AdminPage margin override. |
| projects/plugins/jetpack/_inc/client/components/settings-admin-page/index.jsx | New AdminPage wrapper and footer menu construction for Settings routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| splitHash = splitUrl[ 1 ].split( '?' ); | ||
|
|
||
| searchForTerm( keywords ); | ||
| const searchURL = '#' + splitHash[ 0 ] + ( keywords ? '?term=' + keywords : '' ); |
There was a problem hiding this comment.
doSearch builds ?term= by concatenating raw keywords into the URL. If the term contains reserved characters (e.g., &, #, ?), the URL can become malformed and term parsing will break. Encode the term when building the URL (and keep decoding on read).
| const searchURL = '#' + splitHash[ 0 ] + ( keywords ? '?term=' + keywords : '' ); | |
| const encodedTerm = keywords ? encodeURIComponent( keywords ) : ''; | |
| const searchURL = '#' + splitHash[ 0 ] + ( encodedTerm ? '?term=' + encodedTerm : '' ); |
| return ( | ||
| <div className="jp-settings-nav"> | ||
| <QuerySitePlugins /> | ||
| <nav className="jp-settings-nav__tabs">{ tabs }</nav> |
There was a problem hiding this comment.
The <nav> element has no accessible label. Adding an aria-label (e.g., “Jetpack settings sections”) will make this landmark meaningful to screen reader users, especially since it’s a primary navigation within the Settings page layout.
| <nav className="jp-settings-nav__tabs">{ tabs }</nav> | |
| <nav | |
| className="jp-settings-nav__tabs" | |
| aria-label={ __( 'Jetpack settings sections', 'jetpack' ) } | |
| > | |
| { tabs } | |
| </nav> |
| if ( isDevVersion ) { | ||
| menu.push( { | ||
| label: _x( 'Dev Tools', 'Navigation item.', 'jetpack' ), | ||
| role: 'button', | ||
| onKeyDown: onKeyDownCallback( props.doEnableDevCard ), | ||
| onClick: props.doEnableDevCard, | ||
| } ); | ||
| } | ||
|
|
There was a problem hiding this comment.
The footer menu includes a “Dev Tools” item that dispatches enableDevCard(), but DevCard is no longer rendered anywhere on Settings routes (unlike the legacy Footer). This makes the menu item non-functional in dev builds; either render DevCard alongside AdminPage (gated by canDisplayDevCard) or omit the “Dev Tools” menu entry here.
| if ( isDevVersion ) { | |
| menu.push( { | |
| label: _x( 'Dev Tools', 'Navigation item.', 'jetpack' ), | |
| role: 'button', | |
| onKeyDown: onKeyDownCallback( props.doEnableDevCard ), | |
| onClick: props.doEnableDevCard, | |
| } ); | |
| } |
| <Tab | ||
| to="/security" | ||
| label={ _x( 'Security', 'Navigation item.', 'jetpack' ) } | ||
| onClick={ () => trackNavClick( 'security' ) } | ||
| /> |
There was a problem hiding this comment.
NavLink active styling won’t mark any tab active when location.pathname is /settings (which Settings treats as the Security tab for module managers, and as Sharing for some non-manager flows). This is a regression from NavigationSettings, which explicitly treated /settings as selected for those tabs. Consider linking the default tab to /settings (or redirecting /settings to the canonical route) and/or using a custom active predicate that treats /settings as active for the appropriate tab.
| label={ _x( 'Newsletter', 'Navigation item.', 'jetpack' ) } | ||
| onClick={ () => trackNavClick( 'newsletter' ) } | ||
| /> | ||
| ) } |
There was a problem hiding this comment.
The legacy settings nav included a conditional “Reader” tab (wpcom-reader module, hidden on WoA sites). The new tabs omit it, but /reader is still a settings route and Settings still renders the Reader screen. This makes the Reader page unreachable from the tabs on eligible sites; add the Reader tab back with the same conditions (including the WoA-site exclusion).
| ) } | |
| ) } | |
| { hasModules( [ 'wpcom-reader' ] ) && ! window._jetpack_is_woa && ( | |
| <Tab | |
| to="/reader" | |
| label={ _x( 'Reader', 'Navigation item.', 'jetpack' ) } | |
| onClick={ () => trackNavClick( 'reader' ) } | |
| /> | |
| ) } |
| /> | ||
| ) } | ||
| { ( hasModules( [ 'markdown', 'post-by-email', 'infinite-scroll', 'copy-post' ] ) || | ||
| window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.active ) && ( |
There was a problem hiding this comment.
This directly accesses window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.active. Other parts of the client guard this global with optional chaining, suggesting it isn’t always present. If it’s undefined, this will throw and break Settings rendering; please guard the access (e.g., via optional chaining / boolean fallback).
| window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.active ) && ( | |
| !!window?.CUSTOM_CONTENT_TYPE__INITIAL_STATE?.active ) && ( |
There was a problem hiding this comment.
Safe to apply the suggestion
| // Handle search term from URL on initial load. | ||
| const searchFromUrl = ( () => { | ||
| const search = location.search || ''; | ||
| const pairs = search.substr( 1 ).split( '&' ); | ||
| const term = pairs.filter( item => 0 === item.indexOf( 'term=' ) ); |
There was a problem hiding this comment.
Search terms present in the URL on initial load are parsed into searchFromUrl, but the Redux searchTerm state is never updated from it. Previously, NavigationSettings dispatched filterSearch on mount/route changes so deep links like #/security?term=... immediately filtered settings. Add an effect that reads location.search and dispatches searchForTerm to keep filtering behavior consistent.
Code Coverage SummaryCoverage changed in 1 file.
2 files are newly checked for coverage.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. 🤖 Generated with Claude Code - If this code review was useful, please react with 👍. Otherwise, react with 👎. |
CGastrell
left a comment
CGastrell
left a comment
There was a problem hiding this comment.
Great work migrating the Settings page to AdminPage! I've been working on the same migration for Boost (#47493) so I went through this in detail. Here are some findings:
Footer going off-width (horizontal scroll)
The AdminPage component renders two Containers as children of admin-ui's Page:
- Content Container — uses
fluidprop (max-width: none; padding: unset), so.jp-lowerhandles its own spacing viamax-width: 1040px; margin: 32px auto 0 - Footer Container — does NOT use
fluid(max-width: 1088px; padding: 0 24px; margin: 0 auto)
This mismatch causes the footer to exceed the page width, creating horizontal scroll. The content Container defers to .jp-lower for spacing, but the footer Container applies its own constraints that don't align with Jetpack's layout.
The footer Container lives in projects/js-packages/components/components/admin-page/index.tsx:92. One fix path: make the footer Container fluid too, then style .jp-dashboard-footer with the same margins/max-width as .jp-lower. But since AdminPage is shared across products (Backup, Search, Social), this would need to be done carefully — possibly via a new prop, or via a CSS override scoped to .jp-settings-admin-page.
Missing components on settings routes
Comparing the two render branches in main.jsx, the new AdminPage path omits several components that the legacy path renders on settings routes:
JetpackManageBanner—shouldShowJetpackManageBanner()checks if the path is insettingsRoutes, so it should render on settings pages but isn't included in the new branchSupportCard—shouldShowSupportCard()also includes settings routes, but it's missing from the new branch
These look like regressions.
DevCard not rendering
The code comment acknowledges this: AdminPage's footer only supports menu item arrays, so the DevCard component (a floating dev panel) can't be rendered there. The "Dev Tools" footer link still dispatches enableDevCard(), but DevCard itself isn't in the settings route render tree — so clicking it does nothing. Dev-only, but worth a follow-up.
window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.active unguarded
In settings-nav-tabs/index.jsx line 106:
window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.activeIf this global isn't defined, it'll throw. Should use optional chaining:
window.CUSTOM_CONTENT_TYPE__INITIAL_STATE?.activeMargin override approach
The double-class specificity hack (.jp-settings-admin-page.jp-settings-admin-page { margin-left: 0 }) works but is a workaround. The root cause is that Jetpack zeros out #wpcontent { padding-left: 0 } (in admin-static.scss and _main.scss), which removes the 20px that AdminPage's margin-left: -20px is designed to compensate. For Boost, we solved this by removing the padding-left: 0 override — but in Jetpack's case, non-settings routes still need it, so this hack is the right trade-off until all routes use AdminPage.
Looks great overall — the wrapper + tab split is clean, and the routing fork in main.jsx is a pragmatic approach for incremental migration.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .dops-search.is-expanded-to-container { | ||
| right: 24px; | ||
|
|
||
| &.is-open { | ||
| left: 24px; | ||
| width: auto; | ||
| } |
There was a problem hiding this comment.
The search positioning override uses physical right/left properties, which won’t mirror in RTL. Since this tab bar already uses logical properties (padding-inline, margin-inline-end), switch these to logical equivalents (e.g., inset-inline-end and inset-inline-start) so the search stays correctly aligned in RTL layouts.
| .jp-settings-nav__tabs { | ||
| display: flex; | ||
| flex: 1; | ||
| } |
There was a problem hiding this comment.
.jp-settings-nav__tabs is a non-wrapping flex row with many tabs; without any overflow handling or responsive fallback, the tabs will inevitably overflow/clamp on narrower viewports (and SectionNav previously switched to a dropdown when tabs didn’t fit). Consider adding an overflow strategy (e.g., horizontal scroll with overflow-x: auto/scrollbar-gutter, or a dropdown/collapsible pattern) so navigation remains usable on small screens.
| &:hover, | ||
| &:focus { | ||
| box-shadow: none; | ||
| color: #069e08; | ||
| } |
There was a problem hiding this comment.
The tab link styles remove the default focus treatment (box-shadow: none) but don’t add an accessible replacement focus indicator. This can make keyboard focus hard to see. Add a clear :focus-visible (or the existing .dops-accessible-focus pattern used elsewhere) outline/underline style that meets contrast/visibility requirements.
|
Lots of fixed CSS values here that ideally would be variables, and anywhere we have flex -CSS styles, we are likely better off with HStack/Vstack from wp-components or newer Stack from wp-ui. |
I like it, but can you elaborate more on reasoning for this change in the context of header component change, and would it make sense to just right away update using Can you also add screenshots for the PR so it's easier for designer to review? Thanks! |
The Jetpack Settings tabs are route-driven—each tab is a URL (#/security, #/writing, etc.) handled by react-router. TabPanel doesn't play nicely with URL-based navigation out of the box. You'd have to fight it: sync router state → TabPanel state, handle back/forward buttons, prevent TabPanel from managing its own selection, etc. In short, the custom NavLink tabs replicate the existing behavior with minimal risk. Swapping to a core tab component is a valid follow-up, but a different scope. It's worth investigating whether admin-ui's tabs support routing natively, though.
Will do. |
Gotcha! There's a new tab-like component coming up in DS eventually for that, which likely will be how we need to do most tabbing UIs in Jetpack later on. Considering we want to move away from a separate "Settings" dashboard entirely soon (and each setting would live inside each product), it sounds like, for now, what we have sounds good. |
CGastrell
force-pushed
the
update/settings-use-adminpage
branch
from
31c0c7a to
fa070b1
Compare
Follow-up: AdminPage footer Container causes horizontal scroll on viewports ≤ 1280pxWhile reviewing this PR I noticed horizontal scroll on common laptop viewports. The root cause isn't in this PR — it's in the shared The issue:
When the wp-admin content area is narrower than 1136px (viewport ≤ ~1296px, accounting for the 160px sidebar), the footer overflows and creates a horizontal scrollbar. This affects any File: This should be addressed in the shared component — either making the footer Container I'd suggest handling this as a separate PR since it touches the shared component. |
vianasw
force-pushed
the
update/settings-use-adminpage
branch
from
fa070b1 to
09c90f3
Compare
|
Great stuff! A couple of quick notes just looking at the screens above:
|
|
@keoshi I like the search input suggestion, but I'm also thinking if it's worth it now for this page, since we'll be moving away from the Settings collection. So, is what we have good enough until the page is deprecated, or should it be better and more aligned with other UIs meanwhile? |
As Mikael pointed out, not sure if it's worth changing it if we're getting rid of this page. |
The footer Container used a non-fluid layout with max-width: 1088px and padding: 0 24px. With content-box sizing the effective width becomes 1136px, overflowing when the wp-admin content area is narrower. Explicitly set box-sizing: border-box on the footer Container so padding is always included within the max-width, preventing horizontal scroll regardless of inherited box-sizing. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the custom Masthead + Footer rendering on settings routes with the shared AdminPage component from @automattic/jetpack-components. This brings the Settings page in line with other migrated products (Protect, Newsletter, Search, etc.) that use the unified admin-ui Page header. Changes: - Create SettingsAdminPage wrapper component that configures AdminPage with title, footer menu items (Version, Modules, Debug, dev-only tools), and HeaderNav actions (WoA sites only) - Split main.jsx render into two paths: settings routes use SettingsAdminPage, dashboard routes keep existing Masthead - Footer menu items (Version, Modules, Debug, Reset Options, Dev Tools) migrated to AdminPage's optionalMenuItems prop Note: The legacy Footer component also rendered a DevCard (a floating dev diagnostic panel toggled via "Dev Tools"). DevCard is NOT included in this migration because AdminPage's footer only supports menu item arrays. The "Dev Tools" button still appears in the footer but will not toggle DevCard until it is re-implemented separately. This only affects dev environments. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the old Calypso SectionNav/NavTabs navigation with a new SettingsNavTabs component that uses react-router NavLink, matching Protect's tab pattern. Pass tabs via AdminPage's tabs prop so they render between the header and content. Fix margin mismatch: Jetpack's CSS zeros out #wpcontent padding-left, but AdminPage's margin-left: -20px assumes it's still 20px. Override the margin back to 0 for the Settings page wrapper. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add missing Reader tab (wpcom-reader module, hidden on WoA sites)
- Wrap doSearch in useCallback to prevent breaking Search debounce
- Replace hand-rolled URL parsing with useMemo + URLSearchParams
- Use margin-inline-end instead of margin-right for RTL support
- Remove redundant showFooter={true} (already the default)
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix /settings base route not highlighting any tab by adding alsoActiveFor prop to Tab component (Security for admins, Sharing for non-admins) - Dispatch search term to Redux on mount for ?term= deep links, matching old NavigationSettings.onRouteChange behavior - Guard window.CUSTOM_CONTENT_TYPE__INITIAL_STATE.active with optional chaining to prevent throw if undefined - Render DevCard in settings routes when dev version is active - Fix Search component positioning by adding position: relative to .jp-settings-nav and offsetting absolute-positioned search to respect container padding Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Swap magic numbers for Jetpack CSS custom properties where clean matches exist (--jp-green, --font-body, --jp-button-padding, etc.). Extract WordPress/admin-ui colors (#f0f0f0, #1e1e1e) into documented SCSS variables since Jetpack's --jp-gray-* scale uses inverted numbering and has no exact equivalents. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace custom local SCSS variables with proper token sources: - Import gutenberg-base-styles for gb.$gray-100 (nav border) - Use var(--jp-black) for tab text to match rest of settings UI - Use var(--spacing-base) from ThemeProvider instead of misusing --jp-button-padding and --jp-modal-padding-large - Aligns with patterns used in the Protect admin page tabs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
--spacing-base is not globally available in the Jetpack Settings context (it is scoped per-component, not provided by a ThemeProvider). Without a fallback, all calc() expressions using it resolve to 0, collapsing tab spacing entirely. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Prevents malformed URLs when the search term contains reserved characters like &, #, or ?. The read side (URLSearchParams.get) already decodes automatically. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Screen readers see an unlabeled navigation landmark without this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace right/left with inset-inline-end/inset-inline-start to match the logical properties already used elsewhere in this file. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The existing :focus rule removes box-shadow but provides no replacement focus indicator for keyboard users. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace green hover/focus color with subtle opacity change - Set page background to #FCFCFC with min-height to prevent wp-admin beige from bleeding through - Add horizontal scroll on mobile (660px) with hidden scrollbar, tighter spacing, and smaller font for narrow viewports Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Remove :global() CSS Modules syntax (invalid in plain SCSS) and apply background/overflow rules directly. Override #wpbody-content background via :has() so the beige doesn't bleed around the edges. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This reverts commit 6552ea8.
…mation Replace horizontal tab scroll at <=660px with a collapsible dropdown. Shows selected tab name + chevron, expands to vertical list on tap. Search magnifier slides over the dropdown with a smooth CSS transition. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
vianasw
force-pushed
the
update/settings-use-adminpage
branch
from
6552ea8 to
750dd3a
Compare
… scope) Consolidate duplicated tab label translations into a single module-level constant. Replace template-literal class construction with clsx for consistency. Tighten eslint-disable scope to cover only the callbacks that need it. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
|
@vianasw I gave it a quick run testing the actual UI and while it looks good when collapsed, it behaves in a funky way when expanded.
A few of the things that we should try to fix. All sort of related to the same issue with the search position + behavior.
Do you think these are achievable? |
I think we can give it a try and fix everything (or most of it), but since this is the current behavior in production, it's probably better to address it in a separate PR. |
github-actions
bot
removed
the
[Status] Needs Review
Part of JETPACK-1411
Proposed changes:
AdminPagecomponent from@automattic/jetpack-components, replacing the legacyMastheadandFooterwith the standardized pattern used by Protect and other Jetpack admin pages.SettingsNavTabscomponent using react-routerNavLinkto replace the CalypsoSectionNav/NavTabs/NavItemnavigation. Passed via AdminPage'stabsprop so it renders between the header and content.#wpcontentpadding-left, but AdminPage appliesmargin-left: -20pxassuming the default 20px padding still exists. The fix overrides the margin back to 0 for the Settings page wrapper.useCallbackfor the search handler to preserve debounce behavior,useMemofor URL search param parsing, andmargin-inline-endfor RTL support.Other information:
Does this pull request change what data or activity we track or use?
No changes to tracking or data usage.
Screenshots
Testing instructions:
Changelog