Repository navigation
Proof of concept newsletter mode - #50680
davemart-in wants to merge 44 commits into
Conversation
davemart-in
added
Enhancement
|
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
[Package] Newsletter
[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: 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 8 files. Only the first 5 are listed here.
16 files are newly checked for coverage. Only the first 5 are listed here.
Full summary · PHP report · JS report If appropriate, add one of these labels to override the failing coverage check:
Covered by non-unit tests
|
github-actions
Bot
added
[mu wpcom Feature] Write
[Package] Jetpack mu wpcom
4a54a32 to
314b64d
Compare
f8ba8c4 to
cedc182
Compare
| .jetpack-newsletter-home__granularity-option { | ||
| border-radius: 0; | ||
| padding: var(--wpds-dimension-padding-sm) var(--wpds-dimension-padding-lg); | ||
|
|
||
| + .jetpack-newsletter-home__granularity-option { | ||
| border-inline-start: var(--wpds-border-width-xs) solid var(--wpds-color-stroke-surface-neutral-weak); | ||
| } | ||
|
|
||
| &.is-selected { | ||
| background-color: var(--wp-admin-theme-color, #3858e9); | ||
| color: #fff; | ||
| } | ||
| } |
There was a problem hiding this comment.
Could we avoid these Button component overrides? They will introduce bugs in future since you're relying on existing CSS and structure not change.
Alternatively you have "unstyled" variation for Button if needed, but I bet there's a component variation already you can use or adjust the design. :-)
| const fields = useMemo< Field< RecentPost >[] >( | ||
| () => [ | ||
| { | ||
| id: 'media', |
There was a problem hiding this comment.
There's type "media" in Fields as well which gives you bunch of stuff for free.
| </Text> | ||
| { item.date && ( | ||
| <Text variant="body-sm" className="jetpack-newsletter-home__muted"> | ||
| { item.date } |
There was a problem hiding this comment.
Not sure (depends on API) but you might need localizing the date for user's or site's datetime settings.
Could also be on its own row with date type which then give you all the stuff for free, if it works for the design?
| { | ||
| id: 'recipients', | ||
| label: __( 'Recipients', 'jetpack-newsletter' ), | ||
| getValue: ( { item }: { item: RecentPost } ) => item.recipients ?? 0, | ||
| render: ( { item }: { item: RecentPost } ) => ( | ||
| <span>{ item.recipients === null ? EMPTY_VALUE : String( item.recipients ) }</span> | ||
| ), | ||
| enableSorting: false, | ||
| }, |
There was a problem hiding this comment.
You can just use type: 'number' or integer for values like these to simplify and help with number format i18n for thousans etc. Docs
| // A link wearing the primary button's clothes — see the note at its markup | ||
| // for why it is not a `Button`. Uses the admin theme colour, like the rest of | ||
| // the Newsletter Mode chrome. | ||
| .jetpack-newsletter-home__no-posts-cta { | ||
| background-color: var(--wp-admin-theme-color, #3858e9); | ||
| border-radius: var(--wpds-border-radius-sm); | ||
| color: #fff; | ||
| display: inline-block; | ||
| margin-block-start: var(--wpds-dimension-gap-md); | ||
| padding: var(--wpds-dimension-padding-sm) var(--wpds-dimension-padding-lg); | ||
| text-decoration: none; | ||
|
|
||
| &:hover, | ||
| &:focus { | ||
| color: #fff; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This looks pretty hacky. 😅 Also mixes static color, --wpds, and --wp-admin tokens which is really messy.
Let's just use LinkButton I just made for this purpose. Depends on the version bump (#50509), but for a prototype, it's fine to update packages just in the Newsletter package locally.
| .jetpack-newsletter-home__post-title { | ||
| display: flex; | ||
| flex-direction: column; | ||
| gap: var(--wpds-dimension-gap-xs); | ||
| } |
There was a problem hiding this comment.
You can replace this with Stack component.
| .jetpack-newsletter-intro__body { | ||
| display: flex; | ||
| flex-direction: column; | ||
| // The container owns the outer padding now. | ||
| gap: var(--wpds-dimension-gap-md); | ||
| } |
There was a problem hiding this comment.
You can replace this with Stack component.
| .jetpack-newsletter-intro__title { | ||
| font-size: 20px; | ||
| line-height: 1.3; | ||
| margin: 0; | ||
| } |
There was a problem hiding this comment.
Let's use Text component to set typography consistently.
| } | ||
|
|
||
| .jetpack-newsletter-intro__cta { | ||
| margin-block-start: var(--wpds-dimension-gap-lg); |
There was a problem hiding this comment.
Needing margin here looks like could be solved just by using Stack. Possibly same with width: 100%.
| apiRoot={ getSiteData()?.rest_root } | ||
| apiNonce={ getSiteData()?.rest_nonce } |
There was a problem hiding this comment.
You should need this apiRoot/apiNonce setter here and something is wrong if you do. :-)
| // `Tabs.Tab` sizes its box to its label, so the focus ring lands hard against | ||
| // the word with nothing between the two. A little inline padding gives the ring | ||
| // room to breathe; the tabs keep their own spacing from the list's gap, so the | ||
| // row reads the same. | ||
| .jetpack-newsletter__add-subscribers-tabs { | ||
|
|
||
| [role="tab"] { | ||
| padding-inline: var(--wpds-dimension-padding-xs); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Is this a problem elsewhere too in Tabs used for Jetpack admin pages, so a local fix for just Jetpack isn't something we should have?
| // the URL field doesn't stretch. Paired with `.components-modal__frame` — the | ||
| // element `Modal` puts our class on — because WP's own frame width rules | ||
| // are single-class too, so this shouldn't depend on stylesheet order. | ||
| .components-modal__frame.jetpack-newsletter-share { |
There was a problem hiding this comment.
You can't really rely on internal class like .components-modal__frame as stable API, it can change at any time. You can use modal's own size props to pick a size. You could also use Dialog which is newer component and if I remember right also more size options.
| * Ported from Calypso's `packages/launchpad/src/action-components/share-site-modal`, | ||
| * with three deliberate differences: the WhatsApp host is chosen per |
There was a problem hiding this comment.
More of a product decision but it was always odd to me we have two parallel "social sharing" implementations; this and Jetpack Social.
There was a problem hiding this comment.
Better convert png to a more optimal webp file to save some bytes.
| href={ getNewsletterModeScriptData()?.writeUrl ?? 'post-new.php' } | ||
| > | ||
| { __( 'Write your first post', 'jetpack-newsletter' ) } | ||
| </a> |
There was a problem hiding this comment.
Noted in another comment that this was a bit hacky solution. :-)
https://github.com/Automattic/jetpack/pull/50680/changes#r3689431104
| while it actually takes you somewhere. Styled to match instead. */ } | ||
| <a | ||
| className="jetpack-newsletter-home__no-posts-cta" | ||
| href={ getNewsletterModeScriptData()?.writeUrl ?? 'post-new.php' } |
There was a problem hiding this comment.
| import { | ||
| Button, | ||
| Modal, | ||
| __experimentalHStack as HStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis | ||
| __experimentalInputControl as InputControl, // eslint-disable-line @wordpress/no-unsafe-wp-apis | ||
| __experimentalVStack as VStack, // eslint-disable-line @wordpress/no-unsafe-wp-apis | ||
| } from '@wordpress/components'; |
There was a problem hiding this comment.
Recommend swapping to Button, Dialog and Stack from @wordpress/ui instead of using the old components.
| <SocialLogo | ||
| className="jetpack-newsletter-share__service-icon" | ||
| size={ 24 } | ||
| icon={ link.service } | ||
| /> |
There was a problem hiding this comment.
You could implement the icon just with icon prop for @wordpress/components Button, or with <Button.Icon /> for @wordpress/ui Button. Then you don't need the manual styling to handle it, which isn't super sustainable, as the button's own styles can change and the logo suddenly wouldn't fit/look good. Official icon handles it.
| href={ link.href } | ||
| label={ link.title } | ||
| target="_blank" | ||
| rel="noopener noreferrer" |
There was a problem hiding this comment.
No need rel anymore for modern browsers. Gutenberg's ExternalLink and Link don't use it anymore either.
| className="jetpack-newsletter-share__service" | ||
| href={ link.href } | ||
| label={ link.title } | ||
| target="_blank" |
There was a problem hiding this comment.
For _blank opening things you'd need the ↗
LinkButton is prolly the best component here.
|
I would recommend looking deeper into WP Build for providing the sidebar, route transitions, and opening the editor and navigating back to Newsletter dash. Check examples from Gutenberg for site editor and how smoothly it works when navigating around and how it replaces the classic WP Admin. It'll provide you SPA experience for focused UIs like these, provide you with the sidebar as well. Avoids lots of boilerplate, natively supported by WP. |
| * | ||
| * @return The empty state. | ||
| */ | ||
| const NoPosts = (): JSX.Element => ( |
There was a problem hiding this comment.
Take a look at EmptyState component, would that work here?
You can even just pass it to DataViews with empty prop (docs)
| // The mode's own nav is the frame here, so the Jetpack footer would be | ||
| // out of place. This page only ever renders inside the mode, so it needs | ||
| // no condition — unlike the Newsletter page, which is shared. | ||
| showFooter={ false } |
There was a problem hiding this comment.
Product decision, of course, but I'd include the Footer. Matt directly gave feedback about Jetpack that it was looking really random (page content widths weren't consistent, headers were all over the place, and some pages had footer sand some didn't).
I don't understand the "out of place" design argument either.
| /** | ||
| * Which view to open on. | ||
| * | ||
| * `?view=` wins over the remembered choice: it is the reliable way in — see the | ||
| * shortcut below — and it makes either state a shareable link. | ||
| * | ||
| * @return The view to render first. | ||
| */ | ||
| const getInitialView = (): DashboardView => { |
There was a problem hiding this comment.
It's better to move these kind of logic to router file instead of stage. You can do redirects there before anything renders, while at stage it's pretty late in the flow.
| <div className="jetpack-newsletter-mode-page"> | ||
| { /* Shown over whichever view is up — it introduces the mode, not a view. */ } | ||
| <IntroModal /> | ||
| { view === 'stats' ? <StatsView /> : <OnboardingView /> } |
There was a problem hiding this comment.
You should use the router and separate routes for these. Now you've kinda implemented your own inside the router, which is a bit of a funky solution and unnecessary.
|
Closing out in favor of Dotcom-only approach. See #50973 |
2 participants
Important
Not for merge. This branch is a proof of concept, built as a cheap way to
test the idea end-to-end in one place. It is not intended to ship as-is.
The work will be broken out into a series of smaller, reviewable PRs. A P2 post
will lay out that breakdown for feedback — please hold detailed code review
until then. This PR is here to look at and click through, not to approve.
Newsletter Mode (experimental spike)
Adds an opt-in, focused Newsletter Mode for the unified Newsletter page. When enabled, it declutters the wp-admin left nav down to the newsletter surfaces — Dashboard, Subscribers, Settings, Comments, Write, Monetize — with a "Newsletters" header and one-click exit back to wp-admin.
Proposed changes
An opt-in, focused Newsletter Mode for the unified Newsletter page, plus the first pass at a Newsletter Dashboard.
Mode flag and opt-in
jetpack_newsletter_mode_availablefilter plus a per-site option — so nothing here is reachable unless it is deliberately switched on.jetpack-newsletter/v1) rather than the shared settings whitelist.Focused workspace
Newsletter Dashboard (new mode-only page)
mod+Jor a?view=query arg.Supporting changes
jetpack-mu-wpcom: a newwpcom_write_back_destinationsfilter so the Write editor's back button can return to the Newsletter page. Registered vetted destinations only — an arbitrary return URL via query string is deliberately not supported. Includes a fallback for environments without that filter.stats-admin: proxy the email overview stats resource.Does this pull request change what data or activity we track or use?
Yes — one new Tracks event:
jetpack_subscribers_share_site, recorded when someone shares the newsletter from the share modal. Has atypeproperty recording the share method (copy,web-share, or the social service name).The Newsletter identity section records the existing
jetpack_newsletter_section_saveevent, which five other settings sections already use — no new event there.Testing instructions
Newsletter Mode is off by default and has no UI until it is made available.
?view=statson the Dashboard URL (ormod+J), and confirm the subscriber chart, email performance, and recent posts render.Turning the toggle back off, or removing the filter, should restore the normal Newsletter page and wp-admin menu with no trace of the mode.