Newsletters: modernize Page / footer / header styles#47399
Newsletters: modernize Page / footer / header styles#47399
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?
|
|
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! |
Code Coverage SummaryCoverage changed in 1 file.
Full summary · PHP report · JS report Coverage check overridden by
I don't care about code coverage for this PR
|
| <style> | ||
| /* Admin menu indicators */ | ||
| ul#adminmenu a.wp-has-current-submenu::after, | ||
| ul#adminmenu > li.current > a.current::after { | ||
| border-right-color: #fff; | ||
| } | ||
| body { background: #fff; } | ||
| </style> |
There was a problem hiding this comment.
WP Boot package applies these (and more) styles in similar manner, so this is future-compatible.
It basically applies white background so that loading state and notice-banners on top look better (otherwise they have grey background).
It also changes menu indicator triangle to be white insteead of grey to match page background.
e3c468c to
295079a
Compare
| } | ||
| // Hello Dolly compatibility fix | ||
| .jetpack_page_jetpack-newsletter #dolly { | ||
| background: gb.$white; |
There was a problem hiding this comment.
There was a problem hiding this comment.
Pull request overview
Modernizes the Jetpack → Newsletters wp-admin settings screen by switching to the @wordpress/admin-ui Page layout and updating section UIs to use WordPress Card components and related styles.
Changes:
- Replace the custom header/container layout with
@wordpress/admin-uiPageand Jetpack footer/notices. - Restyle settings sections using
Card/CardHeader/CardBody/CardFooter, and update SCSS to pull in admin-ui/theme tokens. - Update the PHP render root element id and add required JS dependencies for the new layout packages.
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| projects/packages/newsletter/webpack.config.js | Bundling tweaks for @wordpress/admin-ui (and its CSS) in the build pipeline. |
| projects/packages/newsletter/src/settings/style.scss | Switches styling to admin-ui/theme tokens and adds new page/container layout CSS. |
| projects/packages/newsletter/src/settings/index.tsx | Replaces prior AdminPage/Header layout with Page + Stack-based layout and new root element id. |
| projects/packages/newsletter/src/settings/sections/newsletter-section.tsx | Converts “Newsletter” section markup to Card layout. |
| projects/packages/newsletter/src/settings/sections/subscriptions-section.tsx | Converts “Subscriptions” section markup to Card layout and moves save action to CardFooter. |
| projects/packages/newsletter/src/settings/sections/paid-newsletter-section.tsx | Converts “Paid newsletter” section markup to Card layout. |
| projects/packages/newsletter/src/settings/sections/newsletter-categories-section.tsx | Converts section to Card layout and adjusts field label/description presentation. |
| projects/packages/newsletter/src/settings/sections/email-content-section.tsx | Converts “Email content” section markup to Card layout. |
| projects/packages/newsletter/src/settings/sections/email-byline-section.tsx | Converts “Email byline” section markup to Card layout. |
| projects/packages/newsletter/src/settings/sections/email-sender-settings-section.tsx | Converts “Sender settings” section markup to Card layout and moves save action to CardFooter. |
| projects/packages/newsletter/src/settings/sections/email-reply-to-settings-section.tsx | Converts “Reply-to settings” section markup to Card layout. |
| projects/packages/newsletter/src/settings/sections/welcome-email-section.tsx | Converts “Welcome email message” section markup to Card layout and moves save action to CardFooter. |
| projects/packages/newsletter/src/settings/components/header.tsx | Removes the old custom header component. |
| projects/packages/newsletter/src/settings/components/header.scss | Removes the old custom header styling. |
| projects/packages/newsletter/src/class-settings.php | Updates the root mount element id and injects some page-level CSS. |
| projects/packages/newsletter/package.json | Adds dependencies required for admin-ui Page/theme/ui usage. |
| projects/packages/newsletter/changelog/update-newsletter-page | Adds changelog entry for the layout/style update. |
| pnpm-lock.yaml | Locks new WordPress package dependencies. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| requestMap: { | ||
| // Bundle the package with our assets until WP core exposes wp-admin-ui. | ||
| '@wordpress/admin-ui': { external: false }, | ||
| '@wordpress/admin-ui/build-style/style.css': { external: false }, | ||
| }, |
There was a problem hiding this comment.
@wordpress/admin-ui/build-style/style.css is imported (via src/settings/style.scss), but this webpack config does not add a resolve.alias for that subpath. In this repo, other bundles that import that CSS (e.g. Forms dashboard) alias it to the physical file under node_modules/@wordpress/admin-ui/build-style/style.css to avoid resolution failures caused by package exports. Please add the same alias here (or change the import to a supported entrypoint) so the build doesn’t break.
| <NewsletterPage> | ||
| <Stack gap="md" direction="column" className="jetpack-newsletter-settings"> | ||
| { ! isSimpleSite() && <NewsletterSection data={ data } onChange={ handleAutoSave } /> } | ||
|
|
There was a problem hiding this comment.
NewsletterPage already wraps its children in a <Stack ... className="jetpack-newsletter-settings">, but NewsletterSettingsApp adds another nested Stack with the same class. This likely doubles up layout spacing/max-width/margins and makes future layout changes harder. Consider keeping the layout wrapper in one place (either remove the inner Stack here, or remove it from NewsletterPage).
| /* Admin menu indicators */ | ||
| ul#adminmenu a.wp-has-current-submenu::after, | ||
| ul#adminmenu > li.current > a.current::after { | ||
| border-right-color: #fff; | ||
| } | ||
| body { background: #fff; } |
There was a problem hiding this comment.
The inline <style> block includes global selectors (body, ul#adminmenu ...) which makes the page styling harder to maintain and can have unintended side effects if this markup is ever reused/embedded. Prefer moving these rules into the existing bundled stylesheet and scoping them to the newsletter admin page (e.g., body.jetpack_page_jetpack-newsletter ... or #jetpack-newsletter-wp-admin-app ...).
| /* Admin menu indicators */ | |
| ul#adminmenu a.wp-has-current-submenu::after, | |
| ul#adminmenu > li.current > a.current::after { | |
| border-right-color: #fff; | |
| } | |
| body { background: #fff; } | |
| /* Admin menu indicators scoped to the newsletter admin page */ | |
| body.jetpack_page_jetpack-newsletter ul#adminmenu a.wp-has-current-submenu::after, | |
| body.jetpack_page_jetpack-newsletter ul#adminmenu > li.current > a.current::after { | |
| border-right-color: #fff; | |
| } | |
| body.jetpack_page_jetpack-newsletter { background: #fff; } |
| gravatar={ newsletterScriptData.gravatar } | ||
| displayName={ getScriptData()?.user.current_user?.display_name ?? '' } | ||
| dateExample={ newsletterScriptData.dateExample } | ||
| <Card> |
There was a problem hiding this comment.
These section changes could easily be done in a separate PR to simplify.
| label: __( 'Newsletter categories', 'jetpack-newsletter' ), | ||
| description: __( | ||
| 'Which categories will you use for newsletter subscribers? Select all that apply.', |
There was a problem hiding this comment.
Looks like the field type array doesn't support description like other field types do, but the label was horrendous being so long. Could move to a separate PR.
75b8167 to
6328cef
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 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.
| $content-max-width: 1040px; | ||
|
|
||
| .jetpack-page-container { | ||
| margin-left: -20px; |
There was a problem hiding this comment.
.jetpack-page-container uses a physical margin-left: -20px;, which won’t behave correctly in RTL layouts. The surrounding styles already use logical properties (e.g., inset-inline-start/end), so consider switching this to a logical margin (e.g., margin-inline-start) to keep the layout RTL-safe.
| margin-left: -20px; | |
| margin-inline-start: -20px; |
| @use "@automattic/jetpack-base-styles/gutenberg-base-styles" as gb; | ||
| @import "@wordpress/theme/design-tokens.css"; | ||
| @import "@wordpress/admin-ui/build-style/style.css"; | ||
| @import "@wordpress/dataviews/build-style/style.css"; |
There was a problem hiding this comment.
@wordpress/admin-ui/build-style/style.css is imported here, but newsletter’s webpack config does not define a resolve.alias for that path. In this repo, the Forms dashboard build adds an explicit alias for this exact file (e.g., projects/packages/forms/tools/webpack.config.dashboard.js) to avoid resolution failures due to package export maps. Consider adding the same alias in projects/packages/newsletter/webpack.config.js (or importing via an exported entrypoint) so this stylesheet can be resolved reliably during bundling.
| <style> | ||
| /* Admin menu indicators */ | ||
| ul#adminmenu a.wp-has-current-submenu::after, | ||
| ul#adminmenu > li.current > a.current::after { | ||
| border-right-color: #fff; | ||
| } | ||
| body { background: #fff; } | ||
| </style> |
There was a problem hiding this comment.
This inline <style> block makes global admin CSS changes (body background and #adminmenu indicators) directly in the render output. Since the React app already bundles/enqueues style.scss, consider moving these rules into that stylesheet (scoped to the newsletter admin page body class, e.g. body.jetpack_page_jetpack-newsletter) to keep styling versioned/cached and avoid accumulating inline styles in PHP output.
| <style> | |
| /* Admin menu indicators */ | |
| ul#adminmenu a.wp-has-current-submenu::after, | |
| ul#adminmenu > li.current > a.current::after { | |
| border-right-color: #fff; | |
| } | |
| body { background: #fff; } | |
| </style> |
dsas
left a comment
There was a problem hiding this comment.
Tests nicely - it even fixes an issue with horizontal scrolling at certain screen resolutions.
Part of JETPACK-1358
Modernize Newsletters dashboard layout by using
Pagecomponent from Gutenbergadmin-uipackage.Somewhat experiment to se how a shared component for Jetpack pages might look like; fine to merge as-is here but in next steps we'd abstract it out from Newsletter package and use in other pages as well.
Depends visually on sections update merging first:
Proposed changes:
Before
After
With the Hello Dolly plugin:
With announcement on top:
Other information:
Jetpack product discussion
Does this pull request change what data or activity we track or use?
Testing instructions:
add_filter( 'jetpack_wp_admin_newsletter_settings_enabled', '__return_true' );Changelog