Normalize admin page headers with unified AdminHeader wrapping @wordpress/admin-ui#47313
Normalize admin page headers with unified AdminHeader wrapping @wordpress/admin-ui#47313
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
github-actions
bot
added
[Feature] Publicize
|
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. Starter Plugin 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. Mu Wpcom plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Inspect 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. Wpcomsh plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Automattic For agencies client 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. Classic Theme helper plugin 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. Paypal Payment buttons 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. Wpcloud Sso 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. |
github-actions
bot
added
the
[Status] Needs Author Reply
CGastrell
added
the
Coverage tests to be added later
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified AdminHeader component in @automattic/jetpack-components that standardizes admin page headers across Jetpack products by wrapping the @wordpress/admin-ui Page component. It migrates the publicize/Social admin page as a proof of concept, demonstrating the new architecture while maintaining full backward compatibility.
Changes:
- Adds new
AdminHeadercomponent that provides a consistent header with logo (defaulting to Jetpack bolt) and title composition - Extends
AdminPagewith new props (title,subTitle,logo,actions,tabs) for the unified header pattern while preserving the legacyheaderprop for backward compatibility - Migrates publicize admin page to use the new pattern, removing the old custom
AdminPageHeadercomponent and associated files
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
projects/js-packages/components/components/admin-header/index.tsx |
New AdminHeader component that wraps @wordpress/admin-ui Page with Jetpack-specific defaults |
projects/js-packages/components/components/admin-header/types.ts |
TypeScript types for AdminHeader props |
projects/js-packages/components/index.ts |
Exports the new AdminHeader component |
projects/js-packages/components/components/admin-page/types.ts |
Adds new props for unified header support, deprecates legacy header prop |
projects/js-packages/components/components/admin-page/index.tsx |
Implements conditional rendering logic for unified vs legacy header |
projects/js-packages/components/package.json |
Adds @wordpress/admin-ui@1.8.0 dependency |
projects/js-packages/components/changelog/update-normalize-admin-page-headers |
Documents minor addition of AdminHeader component |
projects/packages/publicize/_inc/components/admin-page/index.tsx |
Migrates to use title prop and actions for license text |
projects/packages/publicize/_inc/components/admin-page/page-header/index.jsx |
Deleted - old custom header component |
projects/packages/publicize/_inc/components/admin-page/page-header/logo.js |
Deleted - inline SVG logo |
projects/packages/publicize/_inc/components/admin-page/page-header/styles.module.scss |
Deleted - old header styles |
projects/packages/publicize/_inc/components/admin-page/test/page-header.test.jsx |
Deleted - tests for old header component |
projects/packages/publicize/changelog/update-normalize-admin-page-headers |
Documents patch-level change for header migration |
pnpm-lock.yaml |
Updates lock file with new @wordpress/admin-ui dependency |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
projects/packages/publicize/_inc/components/admin-page/index.tsx
Outdated
Show resolved
Hide resolved
| // should we add a subTitle? Page seems already crowded | ||
| // subTitle={ __( 'Share your posts with your social media network.', 'jetpack-publicize-pkg' ) } |
There was a problem hiding this comment.
The commented-out code suggests uncertainty about whether to include a subtitle. If this is intentional for future consideration, consider converting it to a TODO comment with context. If it's not needed, remove it entirely to reduce code clutter.
| // should we add a subTitle? Page seems already crowded | |
| // subTitle={ __( 'Share your posts with your social media network.', 'jetpack-publicize-pkg' ) } | |
| // TODO: Consider adding a subtitle here if the page layout is simplified in the future. |
| const licenseAction = | ||
| ! hasSocialPaidFeatures() && isJetpackSite | ||
| ? createInterpolateElement( | ||
| __( | ||
| 'Already have an existing plan or license key? <a>Click here to get started</a>', | ||
| 'jetpack-publicize-pkg' | ||
| ), | ||
| { | ||
| a: <a href={ getMyJetpackUrl( '#/add-license' ) } />, | ||
| } | ||
| ) | ||
| : null; |
There was a problem hiding this comment.
The deleted test file page-header.test.jsx contained tests for the license text functionality (showing/hiding based on paid features and site type). These tests should be migrated to ensure the license action behavior is still covered. Consider adding test cases to social-admin-page.test.jsx to verify that the license action appears in the header's actions prop when appropriate.
Code Coverage SummaryCoverage changed in 7 files. Only the first 5 are listed here.
Full summary · PHP report · JS report Coverage check overridden by
Coverage tests to be added later
|
CGastrell
added
[Status] In Progress
and removed
[Status] Needs Author Reply
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 14 changed files in this pull request and generated 5 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const AdminHeader: FC< AdminHeaderProps > = ( { | ||
| logo, | ||
| title, | ||
| subTitle, | ||
| actions, | ||
| tabs = null, | ||
| className, | ||
| breadcrumbs = null, | ||
| badges = null, | ||
| } ) => { | ||
| const classes = className; | ||
|
|
||
| // While admin-ui Page has a title prop, it fails to render both the logo and | ||
| // text. Internally it tries to accommodate both inside Heading. | ||
| // Composing here with Heading as it is on admin-ui Page. | ||
| const composedTitle = title ? ( | ||
| <HStack spacing={ 2 } justify="left"> | ||
| { logo || <JetpackLogo showText={ false } height={ 20 } /> } | ||
| <Heading as="h2" level={ 3 } weight={ 500 } truncate> | ||
| { title } | ||
| </Heading> | ||
| </HStack> | ||
| ) : undefined; | ||
|
|
||
| return ( | ||
| <Page | ||
| className={ classes } | ||
| title={ composedTitle } | ||
| subTitle={ subTitle } | ||
| actions={ actions } | ||
| breadcrumbs={ breadcrumbs } | ||
| badges={ badges } | ||
| showSidebarToggle={ false } | ||
| > | ||
| { tabs } | ||
| </Page> | ||
| ); | ||
| }; | ||
|
|
||
| export default AdminHeader; |
There was a problem hiding this comment.
The new AdminHeader component is introduced without any test coverage. While the old AdminPageHeader test was deleted (which is appropriate since that component was removed), the new component should have tests to verify its prop handling, logo defaulting behavior, and integration with the @wordpress/admin-ui Page component. Consider adding tests for AdminHeader similar to the test coverage that existed for the old AdminPageHeader.
| const licenseAction = | ||
| ! hasSocialPaidFeatures() && isJetpackSite | ||
| ? createInterpolateElement( | ||
| __( | ||
| 'Already have an existing plan or license key? <a>Click here to get started</a>', | ||
| 'jetpack-publicize-pkg' | ||
| ), | ||
| { | ||
| a: <a href={ getMyJetpackUrl( '#/add-license' ) } />, | ||
| } | ||
| ) | ||
| : null; |
There was a problem hiding this comment.
The deleted page-header.test.jsx file contained tests verifying that the license action text ("Already have an existing plan or license key?") appears conditionally based on whether the site has paid features and whether it's a Jetpack site. This behavior is now implemented in the licenseAction variable (lines 83-94) and passed as the actions prop, but there's no test coverage for this functionality in the remaining test files. Consider adding tests to social-admin-page.test.jsx to verify the license action appears/disappears under the correct conditions.
| return ( | ||
| <Page | ||
| className={ classes } | ||
| title={ composedTitle } | ||
| subTitle={ subTitle } | ||
| actions={ actions } | ||
| breadcrumbs={ breadcrumbs } | ||
| badges={ badges } | ||
| showSidebarToggle={ false } | ||
| > | ||
| { tabs } | ||
| </Page> | ||
| ); |
There was a problem hiding this comment.
The @wordpress/admin-ui Page component is designed to wrap the entire page content (header + body), as seen in its usage throughout the forms package. However, AdminHeader uses Page only for the header portion, with only tabs passed as children (line 53), while the actual page content is rendered separately in AdminPage (line 102). This creates an unusual architecture where Page doesn't contain the page content. Consider either: (1) refactoring to have Page wrap all content, or (2) using a more direct approach to render just the header elements without wrapping them in the full Page component. The current approach may cause issues if @wordpress/admin-ui expects Page to manage layout for its entire content area.
| // text. Internally it tries to accommodate both inside Heading. | ||
| // Composing here with Heading as it is on admin-ui Page. | ||
| const composedTitle = title ? ( | ||
| <HStack spacing={ 2 } justify="left"> |
There was a problem hiding this comment.
The HStack uses justify="left", but the codebase more commonly uses justify="start" for left alignment (see examples in forms/routes/responses/integrations-modal.tsx:184, forms/src/blocks/contact-form/components/jetpack-integrations-modal/helpers/akismet.tsx:76, and many others). While both work, using justify="start" would be more consistent with the rest of the codebase.
| <HStack spacing={ 2 } justify="left"> | |
| <HStack spacing={ 2 } justify="start"> |
There was a problem hiding this comment.
alignment="center" might also be needed.
| * Importing the CSS via @import inside a .css file avoids the | ||
| * @wordpress/dependency-extraction-webpack-plugin externalization that | ||
| * breaks direct JS `import '@wordpress/admin-ui/build-style/style.css'`. | ||
| * css-loader resolves @import independently of the externals plugin. |
There was a problem hiding this comment.
You should handle any externalization/bundling issues instead in the Webpack config, which is much more traceable in future when we want to make changes to this. CSS file is otherwise a bit of a hack and mystery change that will go unnoticed in future.
There are existing examples in the repo you can see for dataviews, ui and admin-ui packages.
There was a problem hiding this comment.
I see that forms solves this with resolve.alias + DependencyExtractionPlugin { requestMap: { external: false } }. Could we do this once at @automattic/jetpack-webpack-config and have it for free for each consumer? Not sure if possible, but otherwise we have to do this for every consumer.
There was a problem hiding this comment.
Responding myself. The resolve.alias for the CSS file can't be centralized since the path to the style.css is different for each package—can't hardcode a path that works everywhere.
So the realistic "fix it once" approach would be:
- Add @wordpress/admin-ui: { external: false } to jetpack-webpack-config's defaultRequestMap
- Keep the CSS import in jetpack-components's AdminHeader (or AdminPage)
- Add a sideEffects: true rule for admin-ui CSS in jetpack-webpack-config's default rules
Not sure about going this route right now. I'll prototype and see if it doesn't get out of hand, and otherwise we'll go with the way it's done in Forms.
There was a problem hiding this comment.
Could we do this once at @automattic/jetpack-webpack-config
Yep, totally!
The new newsletter dashboard had this configured as well, I think.
Just note that dependency extraction plugin should already bundle admin-ui. I'm just a bit vague if that applies only to JS, and we still separately need to instruct scss pipeline to bundle the CSS.
There was a problem hiding this comment.
This was a long fight to get it working. I'd like to get it through some webpack config, but when I tried it failed to deliver. Let me know @vianasw if you get somewhere with this.
While I don't like it, it works great as a transitional component.
There was a problem hiding this comment.
What was the issue, and did you check with the Triforce team? Let's not settle on poor workarounds.
| </Col> | ||
| </Container> | ||
| { showHeader && title ? ( | ||
| <AdminHeader |
There was a problem hiding this comment.
I'm not sure I'd componentize what was already a component — seems more complicated than need, and instead just use it directly:
<Page
className={ classes }
title={ composedTitle }
subTitle={ subTitle }
actions={ actions }
breadcrumbs={ breadcrumbs }
badges={ badges }
showSidebarToggle={ false }
>
{ tabs }
</Page>There was a problem hiding this comment.
The rationale for a separate AdminHeader component was to use it independently by products with legacy layouts that can't wrap everything in AdminPage. But in practice, when I tried to use it in the Settings page, the AdminHeader standalone created layout conflicts because it wraps admin-ui's Page component, which is a full-page layout (nested layout systems fighting each other). I'll give it some more thought and see if we can't easily use AdminHeader as a standalone component.
There was a problem hiding this comment.
Alternatively, if you wanna proceed without full Page component as a first step:
- See if the Header can be exported from the admin-ui in upstream outside the Page. (Minding the etiquette pb6Nl-dyI-p2)
- Copy the Header component from Gutenberg locally (just temporarily), which lets you update headers first, and gives you time to work through changes needed for a more complete
<Page>migration.
There's actually old temporary copy for forms still here, but it might be worth taking a fresh one.
There was a problem hiding this comment.
For an exception on one product the full copy Header dance might be good, but not if we have to standardize it for multiple ones. The thin wrapper is meant to cope with that. While it wraps the full Page component, we only use the header support in it while also exporting a standalone version of the AdminHeader. There are some cases where the Page component can easily wrap the legacy (PR) content and we'll use it. I don't see the need to depend on GB exporting admin-ui/header, it would be good though, but I'm unaware of the side effects and requirements. Meanwhile the thin wrapper can deal with it and maintain a flexible back/future compatibility.
Pushing the upstream export should be done, but attaching this work to depend on that is double effort from my PoV.
There was a problem hiding this comment.
@CGastrell I'm not sure I understand what you are suggesting on how we proceed. Can you elaborate?
To be clear, I said above that none of this is a blocker, but getting to a good place in follow-ups is an absolute must so that we don't leave things in a weird state.
| const composedTitle = title ? ( | ||
| <HStack spacing={ 2 } justify="left"> | ||
| { logo || <JetpackLogo showText={ false } height={ 20 } /> } | ||
| <Heading as="h2" level={ 3 } weight={ 500 } truncate> |
There was a problem hiding this comment.
Won't this end up double-wrapping the heading, and locking to pre-defined styles? I'd just expect a plain string and let the actual Header component deal with styles.
There was a problem hiding this comment.
Clarifying that I expect HStack+logo but extra h2 with styles was surprising.
Just noting that we're sorting out a small bug with non-string title but I don't think it's a blocker here.
There was a problem hiding this comment.
You're right. I'll see how to fix this
There was a problem hiding this comment.
Clarifying that I expect
HStack+logo but extrah2with styles was surprising.Just noting that we're sorting out a small bug with non-string
titlebut I don't think it's a blocker here.
got it.
There was a problem hiding this comment.
Won't this end up double-wrapping the heading
Yes, but no harm in it. Meanwhile this is the exact structure used by admin-ui/header. The issue here was that passing the logo as part of title wouldn't guarantee vertical alignment, so structuring the logo and the text (heading) on a HStack seemed like the cleanest way.
When I tried it as simply injecting the title side by side with the logo, alignment would have required custom CSS. This approach nests a heading but achieves the goal with less customizations/hacks.
There was a problem hiding this comment.
@CGastrell you're misunderstanding. HStack is fine, my question was if you end up with <h2><h2>Title</h2></h2>.
| showSidebarToggle={ false } | ||
| > | ||
| { tabs } | ||
| </Page> |
There was a problem hiding this comment.
Might be fine as a stopgap, but now this new Header component locks us too much into the <Page><Header/><Tabs/></Page> format, while right next up, we should wrap the entire page contents with Page instead. How would it look if we used Page for the full page contents already now?
There was a problem hiding this comment.
Doing it "properly" now is a bigger lift per product. The stopgap approach (current AdminHeader) lets products adopt the new header with minimal changes (like the one-line My Jetpack PR), while the full-Page approach requires rethinking each product's layout container hierarchy. Unless we went with the full-page wrapping where we already can (Search, Social, Protect, etc), and do a custom header-only solution for the legacy layouts—just a styled div with the right typography and spacing.
There was a problem hiding this comment.
We still don't have a use for this, we could remove it. But I'd like to see how the Settings page is handling its tabs. This was mostly copied from what we have in forms, that I used as the most refined version.
Removing it should be fine if we don't see a use case for it.
simison
left a comment
simison
left a comment
There was a problem hiding this comment.
Left more detailed comments, but overall as a summary:
- I like how it allows conditionally opting into the new layout page by page.
- I didn’t like the CSS-import hack instead of just using the webpack config as we do elsewhere in the repo.
<Page><Header/><Tabs/></Page>structure, which leaves out the actual page contents, is questionable, but fine as a stop-gap to keep moving if we right away follow up with more appropriate use.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { showHeader && title ? ( | ||
| <AdminHeader | ||
| logo={ logo } | ||
| title={ title } | ||
| subTitle={ subTitle } | ||
| actions={ actions } | ||
| tabs={ tabs } | ||
| /> | ||
| ) : ( |
There was a problem hiding this comment.
When title is provided, AdminPage renders the new AdminHeader path, but the sandboxedDomain badge/testConnection affordance is only rendered in the legacy header branch. As a result, sandboxedDomain becomes a no-op for pages adopting the unified header. Consider rendering the sandbox badge in both branches (e.g., as part of actions, or an additional element in AdminHeader) so existing diagnostics keep working after migrations.
| subTitle, | ||
| logo, | ||
| actions, | ||
| tabs, |
There was a problem hiding this comment.
AdminHeader supports breadcrumbs/badges, but AdminPage doesn’t currently accept/forward them in the unified-header path. If AdminPage is intended to be the single entrypoint for the new header API, consider adding breadcrumbs and badges to the props destructuring here and passing them through to .
| tabs, | |
| tabs, | |
| breadcrumbs, | |
| badges, |
| for await ( const filePath of fs.glob( '**/*.{scss,css}', { | ||
| cwd: packageRoot, | ||
| exclude: Array.from( IGNORED_DIRS, dir => `**/${ dir }/**` ), | ||
| } ) ) { |
There was a problem hiding this comment.
This script now copies both .scss and .css files into build output, but the script name and npm script key are still copy-scss. Consider renaming to something like copy-styles (or adding a brief comment near the script definition) to avoid confusion for future maintainers.
| */ | ||
| tabs?: ReactNode; | ||
|
|
||
| /** |
There was a problem hiding this comment.
AdminPageProps exposes the new unified-header props, but it’s missing breadcrumbs and badges even though AdminHeader (and the PR description) supports them. This prevents consumers from passing these through via AdminPage and makes the public API incomplete. Consider adding breadcrumbs?: ReactNode and badges?: ReactNode to AdminPageProps (and wiring them through in AdminPage) or updating the PR description/props to match the actual supported surface.
| /** | |
| /** | |
| * Breadcrumb navigation displayed in the unified header. | |
| */ | |
| breadcrumbs?: ReactNode; | |
| /** | |
| * Badge elements displayed alongside the title in the unified header. | |
| */ | |
| badges?: ReactNode; | |
| /** |
Add flex column layout with 8px gap to match admin-ui Page header's spacing between title and subtitle. Also add padding-block-end: 8px on subtitle to match the admin-ui-page__header-subtitle style. This brings the total header height to 96px, matching Social, Backup, etc. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 81 out of 82 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| > | ||
| { notice && <Notice floating={ true } dismissable={ true } { ...notice } /> } | ||
| <Container horizontalSpacing={ 0 }> | ||
| title={ __( 'Protect', 'jetpack-protect' ) } |
There was a problem hiding this comment.
The product name "Protect" is passed to title inconsistently: protect/src/js/routes/setup/index.jsx uses the raw string literal 'Protect' with a "do not translate" comment (consistent with the pattern used by "Newsletter" and "Boost"), while protect/src/js/components/admin-page/index.jsx wraps it with __( 'Protect', 'jetpack-protect' ). Product names that are not to be translated should consistently use raw string literals without __() to avoid them being flagged for translation.
| <Logo /> | ||
| <JetpackLogo showText={ false } height={ 20 } /> | ||
| </div> | ||
| <h2 className={ clsx( styles.title ) }> |
There was a problem hiding this comment.
"Boost" is a product name that should not be translated according to the pattern established by "Newsletter", "Protect" (setup page), and "VideoPress" in this PR, which use raw string literals with "do not translate" comments. Wrapping "Boost" with __() makes it appear in translation tools as a translatable string. It should be { 'Boost' /** "Boost" is a product name, do not translate. */ } instead.
The #jb-admin-settings React mount point had a transparent background, so the area behind the floated #dolly element showed the gray wp-admin background instead of white. Give the mount point a white background to match how AdminPage-based products handle this. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The existing #dolly style had a border-bottom that created a visible horizontal line between the Hello Dolly text and the Jetpack header. Remove it so the dolly area blends seamlessly with the header below. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous commit fixed border-bottom in _main.scss (.jetpack-pagestyles #dolly), but admin-static.scss has a duplicate .wp-admin #dolly rule that loads later in the CSS bundle, overriding the fix via cascade order. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| showFooter | ||
| moduleName={ __( 'VaultPress Backup', 'jetpack-backup-pkg' ) } | ||
| header={ <Header /> } | ||
| title={ __( 'Backup', 'jetpack-backup-pkg' ) } |
| <AdminPage | ||
| moduleName={ moduleName } | ||
| showHeader={ false } | ||
| title={ __( 'Social', 'jetpack-publicize-pkg' ) } |
| </div> | ||
| <AdminPage | ||
| moduleName={ __( 'Jetpack Search', 'jetpack-search-pkg' ) } | ||
| title={ __( 'Search', 'jetpack-search-pkg' ) } |
| <Col lg={ 12 } md={ 12 } sm={ 12 }> | ||
| <ConnectionError /> | ||
| <AdminPage | ||
| title={ __( 'Search', 'jetpack-search-pkg' ) } |
| <AdminPage | ||
| moduleName={ __( 'Jetpack VideoPress', 'jetpack-videopress-pkg' ) } | ||
| header={ <JetpackVideoPressLogo /> } | ||
| title={ __( 'VideoPress', 'jetpack-videopress-pkg' ) } |
| } elseif ( 'jetpack_about' === $page ) { | ||
| esc_html_e( 'About', 'jetpack' ); | ||
| } else { | ||
| esc_html_e( 'Jetpack', 'jetpack' ); |
| > | ||
| { notice && <Notice floating={ true } dismissable={ true } { ...notice } /> } | ||
| <Container horizontalSpacing={ 0 }> | ||
| title={ __( 'Protect', 'jetpack-protect' ) } |
Product names (Boost, Protect, Search, Social, Backup, VideoPress, Jetpack, etc.) are brand names and should not be wrapped in translation functions. - Remove moduleName props from AdminPage/JetpackFooter callers so the default "Jetpack" kicks in for footers - Change translated title props to plain strings with do-not-translate comments - Fix default moduleName in AdminPage and JetpackFooter components to use plain strings instead of __() - Fix translated product names in Boost image-guide PHP and Jetpack Stats banner Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Newsletter: remove moduleName prop and MODULE_NAME constant so the default "Jetpack" footer kicks in - Jetpack admin page PHP: use plain echo for "Jetpack" product name instead of esc_html_e() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 87 out of 88 changed files in this pull request and generated no new comments.
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
added
[Status] UI Changes
Part of JETPACK-1353
Somewhat related to JETPACK-1360 as this centralizes the use of Page through AdminHeader Jetpack component.
Summary
Introduces a unified
AdminHeadercomponent in@automattic/jetpack-componentsthat wraps@wordpress/admin-ui'sPagecomponent, providing a consistent header across all Jetpack admin pages. Migrates publicize/Social as the first proof of concept.What this PR does
AdminHeadercomponent to jetpack-components — a thin wrapper around@wordpress/admin-uiPagethat composes a logo + title into the header. Defaults to the Jetpack bolt icon when no custom logo is provided.AdminPageprops withtitle,subTitle,logo,actions,tabs,breadcrumbs,badges. Whentitleis present, renders the newAdminHeader; otherwise falls back to the legacyheaderprop path. Fully backward compatible.title="Social"instead of the customAdminPageHeadercomponent. Deletes the now-unusedpage-header/directory (inline SVG logo, styles, component, test).Social
VideoPress
Backup
Search
Long-term plan
This is a step in the p1HpG7-xCB-p2 effort (i1 designs by @keoshi and @sanjagrbic). Figma: fBCpFRsWEkIP0rx9AXNIJt-fi-4580_27730
Architecture:
AdminHeaderadds only two things that@wordpress/admin-uiPage doesn't have:logoprop is providedMigration pattern for each remaining product:
title="ProductName"toAdminPage(optionallysubTitle,actions,tabs)Products to migrate next (~13 remaining):
<Header>with logo SVGtitlepropheader={<JetpackVideoPressLogo/>}<Header>with logo<Header>with logo + title<JetpackLogo/>fallbackheader={<JetpackProtectLogo/>}HeaderMastheadclass componentFuture convergence: Once all products use
AdminHeader, the wrapper itself can eventually be removed in favor of using@wordpress/admin-uiPagedirectly — the props already map 1:1.Does this pull request change what data or activity we track or use?
No
Testing instructions:
jetpack build packages/publicizepassesjetpack test js packages/publicizepassesjetpack test php packages/publicizepassesChangelog
🤖 Generated with Claude Code