Componentry: migrate ContextualUpgradeTrigger to @wordpress/ui Notice#48909
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! 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. 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. |
Code Coverage SummaryCoverage changed in 1 file.
|
The CUT → Notice migration in social-module-toggle pulled in @wordpress/ui's Tooltip composition, which transitively chains through @wordpress/theme's private-apis. social-admin-page.asset.php now declares wp-theme and wp-private-apis as deps — neither is in WP core, both are polyfilled by wp-build-polyfills. class-social-admin-page.php was enqueuing social-admin-page directly without first registering the polyfills, so the new handles 404'd and the browser threw 'SyntaxError: Unexpected token <' parsing the HTML error page as JS. React tree never mounted, Social e2e times out at the onboarding 'Get Started' button. Pattern matches what videopress / forms / scan / podcast / newsletter / backup already do.
…ess/ui Tooltip The wp-theme polyfill registration approach didn't take effect in publicize's older Assets::register_script pipeline. After diagnosis: no consumer in the codebase has shipped a working @wordpress/ui Tooltip in a non-wp-build plugin yet. Boost (the closest precedent) keeps IconTooltip from jetpack-components for the same reason. Revert the wp-build-polyfills wiring + lockfile regen. Use IconTooltip in social-module-toggle's upgrade Notice. Preserves click-toggle UX from the original CUT tooltipText. IconTooltip migration becomes a separate slice later (item #7 in the shadow analysis priority).
de4324e to
b3ceac3
Compare
…s a11y speak The Notice composition triggers @wordpress/a11y speak() which renders the description text into a visually-hidden .a11y-speak-region in document.body. The existing getByText/queryByText assertions matched both the visible Notice and the speak region, causing 'multiple elements found' on the positive case and 'expected not in document, found' on the negative case (because the speak region persists across jsdom renders). Fix: pass the testing-library ignore option to text queries so they target the visible Notice DOM only. Test-only change; no production code touched.
…#48909) * Componentry: migrate ContextualUpgradeTrigger to @wordpress/ui Notice * Publicize: register wp-build-polyfills before social-admin-page enqueue The CUT → Notice migration in social-module-toggle pulled in @wordpress/ui's Tooltip composition, which transitively chains through @wordpress/theme's private-apis. social-admin-page.asset.php now declares wp-theme and wp-private-apis as deps — neither is in WP core, both are polyfilled by wp-build-polyfills. class-social-admin-page.php was enqueuing social-admin-page directly without first registering the polyfills, so the new handles 404'd and the browser threw 'SyntaxError: Unexpected token <' parsing the HTML error page as JS. React tree never mounted, Social e2e times out at the onboarding 'Get Started' button. Pattern matches what videopress / forms / scan / podcast / newsletter / backup already do. * Lockfile: regenerate consumer composer.lock after publicize wp-build-polyfills dep * Publicize: use IconTooltip from jetpack-components instead of @wordpress/ui Tooltip The wp-theme polyfill registration approach didn't take effect in publicize's older Assets::register_script pipeline. After diagnosis: no consumer in the codebase has shipped a working @wordpress/ui Tooltip in a non-wp-build plugin yet. Boost (the closest precedent) keeps IconTooltip from jetpack-components for the same reason. Revert the wp-build-polyfills wiring + lockfile regen. Use IconTooltip in social-module-toggle's upgrade Notice. Preserves click-toggle UX from the original CUT tooltipText. IconTooltip migration becomes a separate slice later (item #7 in the shadow analysis priority). * Publicize: update social-module-toggle test for @wordpress/ui Notice's a11y speak The Notice composition triggers @wordpress/a11y speak() which renders the description text into a visually-hidden .a11y-speak-region in document.body. The existing getByText/queryByText assertions matched both the visible Notice and the speak region, causing 'multiple elements found' on the positive case and 'expected not in document, found' on the negative case (because the speak region persists across jsdom renders). Fix: pass the testing-library ignore option to text queries so they target the visible Notice DOM only. Test-only change; no production code touched.
…#48909) * Componentry: migrate ContextualUpgradeTrigger to @wordpress/ui Notice * Publicize: register wp-build-polyfills before social-admin-page enqueue The CUT → Notice migration in social-module-toggle pulled in @wordpress/ui's Tooltip composition, which transitively chains through @wordpress/theme's private-apis. social-admin-page.asset.php now declares wp-theme and wp-private-apis as deps — neither is in WP core, both are polyfilled by wp-build-polyfills. class-social-admin-page.php was enqueuing social-admin-page directly without first registering the polyfills, so the new handles 404'd and the browser threw 'SyntaxError: Unexpected token <' parsing the HTML error page as JS. React tree never mounted, Social e2e times out at the onboarding 'Get Started' button. Pattern matches what videopress / forms / scan / podcast / newsletter / backup already do. * Lockfile: regenerate consumer composer.lock after publicize wp-build-polyfills dep * Publicize: use IconTooltip from jetpack-components instead of @wordpress/ui Tooltip The wp-theme polyfill registration approach didn't take effect in publicize's older Assets::register_script pipeline. After diagnosis: no consumer in the codebase has shipped a working @wordpress/ui Tooltip in a non-wp-build plugin yet. Boost (the closest precedent) keeps IconTooltip from jetpack-components for the same reason. Revert the wp-build-polyfills wiring + lockfile regen. Use IconTooltip in social-module-toggle's upgrade Notice. Preserves click-toggle UX from the original CUT tooltipText. IconTooltip migration becomes a separate slice later (item #7 in the shadow analysis priority). * Publicize: update social-module-toggle test for @wordpress/ui Notice's a11y speak The Notice composition triggers @wordpress/a11y speak() which renders the description text into a visually-hidden .a11y-speak-region in document.body. The existing getByText/queryByText assertions matched both the visible Notice and the speak region, causing 'multiple elements found' on the positive case and 'expected not in document, found' on the negative case (because the speak region persists across jsdom renders). Fix: pass the testing-library ignore option to text queries so they target the visible Notice DOM only. Test-only change; no production code touched.
Drop the last 5 migration-relevant @automattic/jetpack-components imports in js-packages/connection. ActionButton's isLoading/displayError/errorMessage behavior is preserved by mapping isLoading -> @wordpress/ui Button's loading prop and rendering the error message as a sibling paragraph with the same jp-action-button__error class. The default error string matches ActionButton's default, so existing tests pass unchanged. Keepers (DecorativeCard, getRedirectUrl, JetpackLogo, PricingCard, TermsOfService) remain on @automattic/jetpack-components; the package dependency stays. Follows #48150 (Publicize), #48489 (My Jetpack interstitials), #49098 (Partner Coupon), and #48909 (CUT migration).
…49099) * Connection: migrate ActionButton, Button, and Text to @wordpress/ui Drop the last 5 migration-relevant @automattic/jetpack-components imports in js-packages/connection. ActionButton's isLoading/displayError/errorMessage behavior is preserved by mapping isLoading -> @wordpress/ui Button's loading prop and rendering the error message as a sibling paragraph with the same jp-action-button__error class. The default error string matches ActionButton's default, so existing tests pass unchanged. Keepers (DecorativeCard, getRedirectUrl, JetpackLogo, PricingCard, TermsOfService) remain on @automattic/jetpack-components; the package dependency stays. Follows #48150 (Publicize), #48489 (My Jetpack interstitials), #49098 (Partner Coupon), and #48909 (CUT migration). * Connection: revert ConnectScreen Basic + RequiredPlan ActionButton migrations These three call sites depend on SCSS rules in `connect-screen/required-plan/style.scss` (lines 46 + 80) and `connect-screen/basic/style.scss` (line 20) that key on the `.jp-action-button--button.components-button` class combination — added by jetpack-components ActionButton and `@wordpress/components` Button. The @wordpress/ui Button doesn't add those classes, so the rules silently stop matching: * RequiredPlan "Get Jetpack Backup" inside PricingCard — lost `width: 100%`, `background: var(--jp-black)`, `color: var(--jp-white)`, padding, border-radius, font-size + font-weight overrides. Rendered as a content-width default-blue `@wordpress/ui` Button instead of the expected full-width black CTA. * RequiredPlan inline "Log in to get started" — lost `display: inline`, `background: inherit`, `text-decoration: underline` text-link treatment. Rendered as a primary-filled rectangular Button inside the otherwise-text paragraph. * Basic "Set up Jetpack" — lost the `.jp-action-button` wrapper + `--button` modifier rules (border-radius, font-weight, responsive max-width). Reverting these three call sites to `ActionButton` preserves the SCSS contract. The connect-button + manage-connection-dialog migrations in this PR stay on `@wordpress/ui` (no SCSS coupling there). A follow-up slice can refactor the SCSS to decouple from the now-internal jetpack class hooks, then re-migrate. * Connection: re-migrate ConnectScreen Basic + RequiredPlan to @wordpress/ui with default styling This supersedes the previous revert (d10b426) and ships the proper @wordpress/ui migration for the three call sites that had been reverted because their SCSS rules were tightly coupled to the old class hooks (`.jp-action-button--button.components-button` and friends). Visual contract follows the user's clarified migration philosophy: default @wordpress/ui Button styling for colors / padding / font-size / border-radius / spinner, with consumer-side SCSS ONLY for structural exceptions (full-width inside the PricingCard, top margin on the basic-screen CTA, responsive max-width). The previous Jetpack-brand overrides (`background: var(--jp-black) !important`, `color: var(--jp-white) !important`) are dropped — WPDS handles those. * **"Get Jetpack Backup" inside PricingCard** (required-plan/visual.jsx): - `@wordpress/ui` Button (default solid + brand), `loading` prop for in-flight feedback (WPDS-styled spinner shows correctly because the button keeps its default chrome). - New stable className `jp-connection__connect-screen__action-button` targeting only `width: 100%` and the vertical rhythm margin. * **"Log in to get started" inline button** (required-plan/visual.jsx): - `@wordpress/ui` Button `variant="unstyled"` styled as an inline text link via SCSS (`.jp-connection__connect-screen__inline-action`). `<button>` element preserves keyboard semantics + a11y; `aria-busy` announces in-flight to screen readers. - `variant="unstyled"` strips Button's own loading visual, so we inject a `@wordpress/components` Spinner via children — matches ActionButton's original "replace label with spinner" behavior. - SCSS aligns the spinner to the prefix-text baseline using `inline-flex` + `align-items: center` + `align-self: baseline`, and zeros out the Spinner's default `5px 11px 0` margin (which existed for the chrome we no longer have). * **"Set up Jetpack" basic CTA** (basic/visual.tsx): - `@wordpress/ui` Button (default solid + brand) + same className hook as RequiredPlan. SCSS keeps only the structural `margin-top: 40px` and the responsive `max-width` breakpoint that goes full-width on mobile. SCSS dead-code removed: * The old `.jp-action-button--button.components-button` selectors at required-plan/style.scss:46 and basic/style.scss:20. * The `.jp-action-button--button.components-button.is-primary` inline-text-link override at required-plan/style.scss:80. * The `.jp-components-spinner__inner / __outer` color overrides at required-plan/style.scss:103. Visually verified in storybook (both Default and Connecting states for the required-plan screen). * Connection: stabilize inline-action width to prevent text-row reflow on loading Adds `min-width: calc(var(--spacing-base) * 8)` and `justify-content: center` to `.jp-connection__connect-screen__inline-action` so the button reserves a fixed minimum width and centers its content. Without this, swapping the children between the (longer) label and the (narrower) spinner during loading caused the surrounding sentence row to reflow visibly. Tracking the user-applied tweak in this commit.
|
@CGastrell This seems to have caused a regression in VideoPress:
It looks quite out of place, is there a way to keep it closer to the original design? |
…f-merge) (#49340) * VideoPress: fix legacy admin crash on upload/delete from base-ui ref-merge The legacy VideoPress admin dashboard crashed with an uncaught TypeError from base-ui@1.4.1's useMergedRefs/useRenderElement path whenever a video was uploaded or deleted (regression from #48909, which migrated UpgradeTrigger to a @wordpress/ui Notice compound tree). UpgradeTrigger conditionally mounted <Notice.Description> based on hasUsedVideo (which flips on upload/delete). That changed the Notice.Root subtree shape across renders; base-ui@1.4.1's useRenderElement conditionally swaps between two different ref-merge hooks (separate useRef slots), so the shifting shape misaligned the stored fork-ref and didChange read .refs off null. Fix: always render <Notice.Description>, varying only its text — mirroring the modern dashboard's free-tier-notice.tsx, which uses the same Notice and never crashes because its Description is static. A non-empty fallback nudge covers the pre-first-upload case so we never render an empty styled row. Keeps the JETPACK-1543 @wordpress/ui Notice migration; does not revert #48909. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * VideoPress: hoist UpgradeTrigger i18n strings to avoid terser ternary-fold The previous fix put both translated strings as the two arms of an inline ternary. Terser folds `cond ? __('a',d) : __('b',d)` into a single `__(cond?'a':'b', d)`, whose msgid is no longer a string literal, so the i18n-check-webpack-plugin fails the production build (makepot can't extract it). Hoist each string to its own module-scope constant so the calls stay separate literal statements. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Maybe it's more of a copy thing than styles. That button is the notice ActionButton in place as intended. Maybe we should take part of the CTA copy outside the button? |
|
Agreed. It might work better as a link |
I fear we might lose visibility with a link. As a notice (that was the best fit candidate I found to replace the contextualupgradetrigger) it holds a space for some text/info and then the action button. I think we could strip part of the CTA out of the button and let the notice do the rest. Turning the button text from:
To notice body + action button:
|
|
Sure, that works |
|
I think either is good! |
|
PR in place, asking for designer guidance there: #49400 |
…nButton (#49400) * VideoPress: replace black Notice.ActionButton upgrade CTA with primary Button The ContextualUpgradeTrigger migration to a @wordpress/ui Notice (#48909) rendered its CTA via Notice.ActionButton, which base-ui styles as a dark/high-contrast button that looks out of place on the VideoPress admin page. Swap it for the standard primary Button already imported from @automattic/jetpack-components, restoring the primary CTA the legacy trigger rendered. The click handler is unchanged, so the jetpack_videopress_upgrade_trigger_link_click Tracks event and the checkout 'run' workflow both still fire. The Notice container and the always-rendered Notice.Description (VIDP-245 base-ui ref-merge invariant) are preserved. * Render VideoPress upgrade CTA as Notice.ActionLink instead of a filled button * VideoPress: restructure upgrade notice as Title + Description + ActionButton Per PR #49400 review (keoshi, simison): replace the long text-link CTA with a concise "Upgrade" Notice.ActionButton, and split the notice copy into two balanced lines — a Notice.Title contextual heads-up and a Notice.Description upsell pitch — following simison's structure example. ActionButton takes onClick directly, so the prior ActionLink href="#" + preventDefault workaround (gutenberg#77098) is dropped entirely. VIDP-245 invariant preserved: Title, Description and Actions are all rendered unconditionally; only the title text varies on hasUsedVideo, keeping the Notice.Root subtree shape stable so base-ui's ref-merge hook does not misalign and crash on the next video upload/delete.







Part of #48160.
Proposed changes
Migrate all 7 in-repo consumers of
ContextualUpgradeTriggeroff@automattic/jetpack-componentsand onto@wordpress/ui'sNoticecomposition. Mark theContextualUpgradeTriggersource as@deprecated(JSDoc only — no API change) following the chip → Badge (#48162) and Status (#48711) precedents. No deletions on the source; npm consumers continue to compile.This is the second slice in the wrapper-first migration plan after Status (#48711). The shadowing analysis identified CUT as the highest impact-per-line wrapper: 44 LOC source, but its migration collapses 6 shadowed
Text+ 2 shadowedButton+ 1 shadowedTitlesites that would otherwise need separate re-touches.Design refresh, not visual parity. The
Noticechrome (rounded, soft-tinted, intent icon at top-left) is intentionally different from the legacy CUT card. This aligns with the broader UI modernization established by #48550 (Search dashboardSimpleNotice→Notice).Per-consumer mapping
plugins/protect(3 sites — co-located helper):src/js/components/threats-list/free-list.jsxsrc/js/routes/scan/scan-footer.jsxsrc/js/routes/firewall/index.jsx<UpgradeNotice>helper atsrc/js/components/upgrade-notice/index.tsx. The helper is a thin shell overNotice.Root+Notice.Description+Notice.Actionsand supports both onClick and href callsites. Following theAdminSectionHero.StatusIndicatorpattern from the Status slice.js-packages/scan(1 site — inline):src/components/threat-modal/threat-fix-details.tsx— inlineNotice.Rootwith anActionButton.packages/videopress(1 site — inline):src/client/admin/components/admin-page/index.tsx— inlineNotice.Rootwith anActionButton. The conditional emptydescriptionshort-circuits cleanly inside the Notice composition.packages/search(1 site — inline):src/dashboard/components/pages/sections/plan-usage-section.jsx— inlineNotice.Rootwith anActionButton. Drops theThemeProviderwrapper (Notice has its own styles).packages/publicize(1 site — inline Notice + IconTooltip):_inc/components/admin-page/toggles/social-module-toggle/index.tsx— only consumer that usedtooltipText. InlineNotice.Rootwith anActionLink, plus anIconTooltipfrom@automattic/jetpack-componentsfor the click-toggle "more details" affordance next to the description. KeepingIconTooltipfrom jetpack-components matches the Boost precedent (plugins/boost/.../page-cache/meta/meta.tsx):@wordpress/uiTooltipchains through@wordpress/themeprivate-apis, which adds awp-themescript handle that the olderAssets::register_scriptpipeline doesn't have polyfill plumbing for.IconTooltipmigration becomes a separate slice later (item Publicize: Add filter to allow plugins to bypass Publicize #7 in the shadow analysis priority).js-packages/components(source —@deprecatedJSDoc only):components/contextual-upgrade-trigger/index.tsx— adds the deprecation block above the export, pointing atNotice. Implementation, styles, stories, and barrel export are unchanged.Test fixtures
packages/search/src/dashboard/components/pages/sections/test/plan-usage-section.test.jsx— replaces the@automattic/jetpack-componentsContextualUpgradeTriggermock with a@wordpress/uiNoticemock that emits the sameupgrade-trigger/upgrade-description/upgrade-ctatestids. All existing assertions pass unchanged.packages/search/tests/js/dashboard/pages/dashboard-page.test.jsx— drops the now-deadContextualUpgradeTriggerentry from the@automattic/jetpack-componentsvirtual mock.Acceptance
grep -rn "from '@automattic/jetpack-components'" projects/ | grep '\bContextualUpgradeTrigger\b'returns zero matches in source files. ✅pnpm jetpack build plugins/protect,pnpm jetpack build packages/videopress --deps,pnpm jetpack build packages/search,pnpm jetpack build packages/publicize,pnpm jetpack build js-packages/scan,pnpm jetpack build js-packages/components. ✅pnpm jetpack test js packages/search,pnpm jetpack test js packages/publicize,pnpm jetpack test js js-packages/scan. ✅jetpack-videopress-pkg/jetpack-search-pkg/jetpack-publicize-pkgare unchanged from trunk). ✅Related product discussion/links
@deprecatedJSDoc precedent.SimpleNotice→@wordpress/uiNotice.js-packages/components/components/chip/index.tsx.Does this pull request change what data or activity we track or use?
No.
Testing instructions
Jetpack → Protect(free plan). The upgrade prompt at the bottom of the page should render as a@wordpress/uiNotice (info intent, rounded, soft-tinted background, info icon, "Upgrade Jetpack Protect now" action button). Click the action — the existing upgrade flow should fire (upgradePlan()viausePlan).Jetpack → Protect → Firewall(free plan). Below the "Automatic firewall protection" toggle, the upgrade prompt should render as a@wordpress/uiNotice. Action button triggers the same upgrade flow.fixedInvalue. The "How to fix it?" panel should show the new Notice with the action button.Jetpack → VideoPresson a site without a VideoPress purchase. The upgrade prompt should render as a Notice with the "Upgrade now to unlock unlimited videos…" action button.Jetpack → Searchon a free Search plan with simulated usage data (overage or near-overage). The plan-usage section should render the upgrade Notice in the appropriate scenario (records, requests, both, or no-overage).Jetpack → Social(or the Jetpack Settings → Sharing tab on a non-WPCOM site without paid Social features). Below the "Automatically share your posts" toggle, the "Unlock advanced sharing options" Notice should render. Click the info icon next to the description — the tooltip with the longer copy should toggle open (click-toggle UX, matching the legacy CUT). Click "Power up Jetpack Social" — it should navigate to the upsell URL.Screenshots
NOTE: the missing screenshots refer to exact same implementation replacements