Connection: migrate ActionButton, Button, and Text to @wordpress/ui#49099
Open
CGastrell wants to merge 4 commits into
Open
Connection: migrate ActionButton, Button, and Text to @wordpress/ui#49099CGastrell wants to merge 4 commits into
CGastrell wants to merge 4 commits into
Conversation
Contributor
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Contributor
|
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 SummaryThis PR did not change code coverage! That could be good or bad, depending on the situation. Everything covered before, and still is? Great! Nothing was covered before? Not so great. 🤷 |
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).
…grations 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.
…ss/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).
…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
force-pushed
the
change/migrate-connection-to-wp-ui
branch
from
dba18a8 to
0b1cca5
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #48160.
Proposed changes
Migrate the remaining 5 migration-relevant
@automattic/jetpack-componentsimports in@automattic/jetpack-connectionto@wordpress/ui, taking the package to zero on the migration leaderboard.ActionButton× 3 →@wordpress/uiButtonButton× 1 →@wordpress/uiButtonText× 1 →@wordpress/uiTextKeepers (
DecorativeCard,getRedirectUrl,JetpackLogo,PricingCard,TermsOfService) remain on@automattic/jetpack-components; the package dependency stays.Props mapping
ActionButtoncall sites inconnection/use more than justlabel+onClick, so this slice exercises the full mapping (unlike #49098, where every call site was trivial):ActionButtonprop@wordpress/uiButton equivalentlabel="…"children(button text)onClick={…}onClick={…}isLoading={…}loading={…}(auto-disables the button + announces via a11y)isDisabled={…}disabled={…}displayError+errorMessage<p className="jp-action-button__error">rendered conditionallyvariant(default"primary")ActionButton's default error string (__( 'An error occurred. Please try again.', 'jetpack-components' )) is reproduced inline with the connection text domain so the existingit( 'shows error into button' )test assertions match unchanged.For the
<Button weight="regular" variant="secondary">call site inmanage-connection-dialog,variant="secondary"maps tovariant="outline"(matching the precedent inscan/src/js/screens/overview/*-modal.tsxandvideopress/src/dashboard/components/video-details/).weight="regular"was the default and has no@wordpress/uiequivalent — dropped.For
Text, both call sites use the defaultvariant="body", which maps cleanly to@wordpress/uiText's defaultvariant="body-md"(verified against #48909).Files changed
components/connect-button/index.jsx—ActionButton(used by RNA connection flow). Error rendered as sibling<p>.components/connect-screen/basic/visual.tsx—ActionButtoninside<ConnectScreen>. PreservesdisplayButtonError || isOfflineModeerror branch with the offline / error-code message.components/connect-screen/required-plan/visual.jsx— twoActionButtoncall sites (main CTA inside<PricingCard>+ "Log in to get started" interpolated link).components/manage-connection-dialog/index.jsx—Button(Cancel in help footer) + twoTextusages.changelog/migrate-jetpack-components-to-wp-ui— patch / changed.@wordpress/ui@0.11.0was already a direct dep — nopackage.jsonchange needed.Acceptance
scan-imports.py) showsjs-packages/connectionmigration-relevant total = 0. ✅pnpm jetpack build js-packages/connectionclean. ✅pnpm --filter @automattic/jetpack-connection test— 12 suites, 70 passed, 2 todo (unchanged from trunk). ✅ The'shows error into button'tests in bothconnect-screen/basic/test/component.jsxandconnect-screen/required-plan/test/component.jsxcontinue to find "An error occurred. Please try again." in the document. ThegetByRole( 'button', { name: ... } )assertions match the new@wordpress/uiButton without test changes.Related product discussion/links
ActionButtonminimal): Partner Coupon: migrate ActionButton to @wordpress/ui #49098Textvariant mapping): Componentry: migrate ContextualUpgradeTrigger to @wordpress/ui Notice #48909Does this pull request change what data or activity we track or use?
No.
Testing instructions
The connection package powers the Jetpack site/user connection flows. Spot-checks I'd suggest reviewers run:
<ConnectScreen>(e.g. a fresh Jetpack install or any flow that drops a user on the basic Connect screen). The "Set up Jetpack" CTA should render as a primary@wordpress/uiButton, with the Terms of Service text immediately above it. Click it — the existing connection flow should trigger.<ConnectScreenRequiredPlan>(e.g. paid Jetpack product onboarding screens with apriceBefore/priceAfter). Confirm the main CTA renders as primary inside<PricingCard>and the "Already have a subscription? Log in to get started" interpolated button still works.displayButtonErrororisOfflineModetruthy at anyConnectScreen*call site — the error paragraph "An error occurred. Please try again." (or the offline-mode interpolated link) should render immediately below the button.siteIsRegistering/userIsConnecting(e.g. click Connect on a fresh site). The button should show the@wordpress/uiButton loading spinner and be disabled.<ManageConnectionDialog>from any consumer (My Jetpack → Connection settings is the easy reach). The "Cancel" button in the help footer should render as an outline-variant@wordpress/uiButton; both<Text>usages (large opening paragraph + help-footer copy) should render with the defaultbody-mdtypography.If a visual reviewer can capture before/after of the connect screen + manage-connection dialog, please drop them on this PR — happy to update the body with the URLs.