Refactor "Use your domain" to functional component by ciampo · Pull Request #101854 · Automattic/wp-calypso

ciampo · 2025-03-25T17:52:50Z

Related to #101833

Proposed Changes

Refactor the React component from class to functional in preparation for the work highlighted in #100243 (comment)

Why are these changes being made?

Refactoring to functional component will allow the usage of the necessary React hooks to complete #100243

Testing Instructions

Visit /domains/add/use-your-domain/{SITE_SLUG} and make sure everything works as on trunk.

Tip

It's highly recommended to review commit-by-commit

Note

I couldn't find a way to visit this page organically. Who would be the best person / team to understand whether this whole branch of domain transfer steps can be deleted?

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
Have you written new tests for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
- For UI changes, have we tested the change in various languages (for example, ES, PT, FR, or DE)? The length of text and words vary significantly between languages.
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

github-actions · 2025-03-25T17:53:05Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-138854

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-138854&env=jetpack

Automattic for Agencies live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-138854&env=a8c-for-agencies

client/components/domains/use-your-domain-step/index.tsx

matticbot · 2025-03-25T18:03:04Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~205 bytes removed 📉 [gzipped])

Details

name     parsed_size           gzip_size
domains      -2787 B  (-0.1%)     -205 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

matticbot · 2025-03-25T18:10:03Z

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

help-center
notifications
wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug refactor/use-your-domain-class-to-functional on your sandbox.

ciampo · 2025-03-26T07:01:46Z

client/components/domains/use-your-domain-step/index.jsx

-	static propTypes = {
-		analyticsSection: PropTypes.string.isRequired,
-		basePath: PropTypes.string,
-		cart: PropTypes.object,
-		domainsWithPlansOnly: PropTypes.bool,
-		primaryWithPlansOnly: PropTypes.bool,
-		goBack: PropTypes.func,
-		initialQuery: PropTypes.string,
-		isSignupStep: PropTypes.bool,
-		mapDomainUrl: PropTypes.string,
-		transferDomainUrl: PropTypes.string,
-		onRegisterDomain: PropTypes.func,
-		onTransferDomain: PropTypes.func,
-		onSave: PropTypes.func,
-		selectedSite: PropTypes.oneOfType( [ PropTypes.object, PropTypes.bool ] ),
-		forcePrecheck: PropTypes.bool,
-	};


I was able to remove a lot of props compared to the previous implementation:

some of them came from connecting the component to the Redux Global state, and were replaced by hooks;

some of them are actually not currently used, and I guess were not cleaned up over time

In the Git history I see big refactorings (#25573 is an example) where large chunks of code were moved around between use-your-domain-step, register-domain-step, transfer-domain-step etc. That explains how many props and state fields got left behind unused.

ciampo · 2025-03-26T07:03:09Z

client/components/domains/use-your-domain-step/index.jsx

-	getDefaultState() {
-		return {
-			authCodeValid: null,
-			domain: null,
-			domainsWithPlansOnly: false,
-			inboundTransferStatus: {},
-			precheck: get( this.props, 'forcePrecheck', false ),
-			searchQuery: this.props.initialQuery || '',
-			submittingAuthCodeCheck: false,
-			submittingAvailability: false,
-			submittingWhois: get( this.props, 'forcePrecheck', false ),
-			supportsPrivacy: false,
-		};
-	}


Most of the old "state" wasn't actually used — in fact, setState is never called in this component. The only state usage that I kept in the new version is for storing the initial value of a prop.

ciampo · 2025-03-26T07:04:25Z

client/components/domains/use-your-domain-step/index.tsx

+		domainsWithPlansOnlyButNoPlan
+	) {
+		transferSalePriceText = translate( 'Sale price is %(cost)s', {
+			args: { cost: domainProductSalePrice! },


Added a ! because isEmpty is not acting as a type guard for domainProductSalePrice

You can remove the isEmpty check (it's lodash, so it's unwanted anyway) and replace it with domainProductSalePrice !== null.

ciampo · 2025-03-26T07:06:30Z

client/components/domains/use-your-domain-step/index.tsx

+		// @ts-expect-error despite the TS error, formatCurrency works with a
+		// `null` currencyCode and uses a fallback currency.
+		mappingPriceText = formatCurrency( price, currencyCode );


I didn't want to work on formatCurrency to prevent scope bloat, but I get the sense, from the number of warnings that I see in the browser console, that formatCurrency is often invoked with a null or undefined currency code. I wonder if we should change it so that it handles null or undefined internally

This TS error can be fixed by changing the if ( price ) condition to if ( price && currencyCode ). The currencyCode Redux state is initialized when the products list is finished fetching (triggered by QueryProducts).

I wonder if we should change it so that it handles null or undefined internally

I think this might be hard to do. How do you format a price if you don't know the currency? Better to not render a price at all if you don't know how but what to render in its place? That's very context specific and probably should be in the hands of the caller.

This TS error can be fixed by changing the if ( price ) condition to if ( price && currencyCode )

I think this is the right solution.

Thank you both for the advice — I'll go ahead and implement the suggested solution, even if it introduces a runtime change.

Better to not render a price at all if you don't know how but what to render in its place?

That's a good point, but one that I see ignored repeatedly across the codebase (judging by the amount of browser console warnings) 😞

I guess that the fact that formatCurrency still picks a default currency also doesn't help.

One thing that could improve the formatCurrency situation is fixing the getDomainPrice function: it should return null if the currencyCode is not valid.

There are just a handful of usages, and the function already returns null when there is no valid "price" value, so hopefully this would be an easy refactoring.

ciampo · 2025-03-26T07:09:18Z

client/components/domains/use-your-domain-step/index.jsx

-				className="use-your-domain-step__option-button"
-				primary={ isPrimary }
-				onClick={ onClick }
-				busy={ submitting }


I removed the busy prop since, in the current implementation, it was always computing as false

ciampo · 2025-03-26T08:36:11Z

client/components/domains/use-your-domain-step/index.tsx

-
-export default connect(
-	( state ) => ( {
-		currentUser: getCurrentUser( state ),


Removed in the new version as it was not used in code

ciampo · 2025-03-26T08:36:39Z

client/components/domains/use-your-domain-step/index.tsx

-		productsList: getProductsList( state ),
-	} ),
-	{
-		errorNotice,


Also removed as I couldn't see any error notice being dispatched in the code

ciampo · 2025-03-26T08:43:01Z

client/components/domains/use-your-domain-step/index.tsx

-		recordTransferButtonClickInUseYourDomain,
-		recordMappingButtonClickInUseYourDomain,


Moved to inline dispatch calls

ciampo · 2025-03-26T08:43:56Z

client/components/domains/use-your-domain-step/index.tsx

-		recordTransferButtonClickInUseYourDomain,
-		recordMappingButtonClickInUseYourDomain,
-	}
-)( withCartKey( withShoppingCart( localize( UseYourDomainStep ) ) ) );


localize => useTranslate()

withShoppingCart => useShoppingCart()

withCartKey => useCartKey()

ciampo · 2025-03-26T08:45:17Z

client/components/domains/use-your-domain-step/index.tsx

-					{ this.renderSelectTransfer() }
-					{ this.renderSelectMapping() }


Refactored these two render functions (and the underlying renderOptionContent) to an internal UseYourDomainStepContent component

ciampo · 2025-03-26T08:49:49Z

client/components/domains/use-your-domain-step/index.tsx

+		! isEmpty( domainProductSalePrice ) &&
+		! isNextDomainFree( cart ) &&
+		! isDomainBundledWithPlan( cart, searchQuery ) &&


Inverted the boolean check since we can't early return in the render function.

ciampo · 2025-03-26T08:50:46Z

client/components/domains/use-your-domain-step/index.tsx

+	if ( domainProductPrice ) {
 		if (
-			isEmpty( domainProductSalePrice ) ||
 			isNextDomainFree( cart ) ||
 			isDomainBundledWithPlan( cart, searchQuery ) ||
-			domainsWithPlansOnlyButNoPlan
-		) {
-			return;
-		}
-
-		return translate( 'Sale price is %(cost)s', { args: { cost: domainProductSalePrice } } );
-	};
-
-	getTransferPriceText = () => {
-		const {
-			cart,
-			currencyCode,
-			translate,
-			domainsWithPlansOnly,
-			isSignupStep,
-			productsList,
-			selectedSite,
-		} = this.props;
-		const { searchQuery } = this.state;
-		const productSlug = getDomainProductSlug( searchQuery );
-		const domainsWithPlansOnlyButNoPlan =
-			domainsWithPlansOnly && ( ( selectedSite && ! isPlan( selectedSite.plan ) ) || isSignupStep );
-
-		const domainProductPrice = getDomainPrice( productSlug, productsList, currencyCode );
-
-		if (
-			domainProductPrice &&
-			( isNextDomainFree( cart ) ||
-				isDomainBundledWithPlan( cart, searchQuery ) ||
-				domainsWithPlansOnlyButNoPlan ||
-				getDomainTransferSalePrice( productSlug, productsList, currencyCode ) )
+			domainsWithPlansOnlyButNoPlan ||
+			getDomainTransferSalePrice( productSlug, productsList, currencyCode )
 		) {
-			return translate( 'Renews at %(cost)s', { args: { cost: domainProductPrice } } );
-		}
-
-		if ( domainProductPrice ) {
-			return translate( '%(cost)s per year', { args: { cost: domainProductPrice } } );
-		}
-	};
-
-	getMappingPriceText = () => {
-		const {
-			cart,
-			currencyCode,
-			domainsWithPlansOnly,
-			primaryWithPlansOnly,
-			productsList,
-			selectedSite,
-			translate,
-		} = this.props;
-		const { searchQuery } = this.state;
-
-		let mappingProductPrice;
-
-		const price = get( productsList, [ 'domain_map', 'cost' ], null );
-		if ( price ) {
-			mappingProductPrice = formatCurrency( price, currencyCode );
-			mappingProductPrice = translate(
-				'%(cost)s per year plus registration costs at your current provider',
-				{ args: { cost: mappingProductPrice } }
-			);
+			transferPriceText = translate( 'Renews at %(cost)s', { args: { cost: domainProductPrice } } );
+		} else {
+			transferPriceText = translate( '%(cost)s per year', { args: { cost: domainProductPrice } } );


Extracted the common domainProductPrice check, and moved the rest of the checks to a nested if...else

ciampo · 2025-03-26T08:52:35Z

client/components/domains/use-your-domain-step/index.tsx

+		);
+	}
+	if (
+		isDomainMappingFree( selectedSite ?? undefined ) ||


Quick fix since selectedSite can be undefined OR null, but isDomainMappingFree doesn't accept null

You can add the | null type to the isDomainMappingFree, it's a change that makes sense.

jsnajdr · 2025-03-26T15:02:00Z

client/components/domains/use-your-domain-step/index.tsx

+						{ reasons.map( ( phrase, index ) => {
+							if ( isEmpty( phrase ) ) {
+								return;
+							}


This isEmpty call be eliminated in favor of phrase === null. The value (member of reason array) is always either null or a result of translate().

jsnajdr · 2025-03-26T15:04:47Z

client/components/domains/use-your-domain-step/index.tsx

+	}
+
+	let mappingPriceText;
+	const price = get( productsList, [ 'domain_map', 'cost' ], null );


This is the only usage of get in this file and can be easily replaced with ?.. Let's completely remove Lodash from here!

Let's! f52f737

jsnajdr

I like this 🙂

jsnajdr · 2025-03-28T11:01:51Z

client/components/domains/use-your-domain-step/index.tsx

+	let mappingPriceText;
+	const price = productsList?.domain_map?.cost;
+	if ( price && currencyCode ) {
+		mappingPriceText = formatCurrency( price, currencyCode );


nit: the formatCurrency result should be assigned to its own variable. Like cost.

af0065c

Totally agree, it's always hard to draw a line when doing these refactors, as there's much more that I'd like to change 😅

jsnajdr · 2025-03-28T11:06:19Z

client/lib/cart-values/cart-items.ts

-export function isDomainMappingFree(
-	selectedSite: undefined | { plan: { product_slug: string } }
-): boolean {
+export function isDomainMappingFree( selectedSite: undefined | SiteDetails | null ): boolean {


nit: the sorting of the | subtypes looks very random. Usually we put the "real" type first:

SiteDetails | null | undefined

This file seems to use some sort of Yoda convention, not sure why.

Yup 8380a7b

… argument

ciampo commented Mar 25, 2025

View reviewed changes

client/components/domains/use-your-domain-step/index.tsx Show resolved Hide resolved

ciampo force-pushed the refactor/use-your-domain-class-to-functional branch 3 times, most recently from e63f690 to 3fd2fd8 Compare March 25, 2025 18:17

ciampo changed the title ~~Refactor "Use your domain" component to functional~~ Refactor "Use your domain" to functional component Mar 25, 2025

ciampo commented Mar 26, 2025

View reviewed changes

ciampo force-pushed the refactor/use-your-domain-class-to-functional branch from 3fd2fd8 to f08ef0f Compare March 26, 2025 08:41

ciampo commented Mar 26, 2025

View reviewed changes

ciampo self-assigned this Mar 26, 2025

ciampo requested review from a team March 26, 2025 09:28

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 26, 2025

ciampo marked this pull request as ready for review March 26, 2025 09:29

ciampo mentioned this pull request Mar 26, 2025

Use your domain: open inline help center #101885

Merged

8 tasks

ciampo added the [Type] Janitorial label Mar 26, 2025

jsnajdr reviewed Mar 26, 2025

View reviewed changes

ciampo force-pushed the refactor/use-your-domain-class-to-functional branch from eebf1db to d637096 Compare March 27, 2025 17:36

ciampo requested review from jsnajdr and sirbrillig March 27, 2025 17:46

jsnajdr approved these changes Mar 28, 2025

View reviewed changes

ciampo added 10 commits March 28, 2025 12:43

Rename to tsx

222ebd5

Refactor from class to functional component

b8e49b8

Use existing SiteDetails type (plus some light refactor)

1c7f538

Remove unused props and simplify

f866063

Refactor extractBasePath function

00df264

Remove lodash

a64594d

Check for currencyCode and remove @ts-expect-error

1c339b7

isDomainMappingFree: accept null as a type for the selectedSite…

c68c9a1

… argument

Avoid unnecessary re-assignment

af0065c

Sort types

8380a7b

ciampo force-pushed the refactor/use-your-domain-class-to-functional branch from d637096 to 8380a7b Compare March 28, 2025 11:45

ciampo merged commit 2eede8c into trunk Mar 28, 2025
13 checks passed

ciampo deleted the refactor/use-your-domain-class-to-functional branch March 28, 2025 12:28

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 28, 2025

		recordTransferButtonClickInUseYourDomain,
		recordMappingButtonClickInUseYourDomain,

		{ this.renderSelectTransfer() }
		{ this.renderSelectMapping() }

Conversation

ciampo commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Why are these changes being made?

Testing Instructions

Pre-merge Checklist

Uh oh!

github-actions bot commented Mar 25, 2025 • edited by matticbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

matticbot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

matticbot commented Mar 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirbrillig Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ciampo Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsnajdr left a comment

Choose a reason for hiding this comment

ciampo commented Mar 25, 2025 •

edited

Loading

github-actions bot commented Mar 25, 2025 •

edited by matticbot

Loading

matticbot commented Mar 25, 2025 •

edited

Loading

matticbot commented Mar 25, 2025 •

edited

Loading

ciampo Mar 26, 2025 •

edited

Loading

sirbrillig Mar 26, 2025 •

edited

Loading

ciampo Mar 27, 2025 •

edited

Loading