Domains: Update domain mapping thank you page #51031

leonardost · 2021-03-12T13:39:00Z

Changes proposed in this Pull Request

In order to improve the user experience when buying domains and mappings, domain mapping instructions were updated in #50668. This PR updates the Thank You page, which is shown after a user buys a domain mapping, to reflect the same instructions that were changed in #50668.

Depending on the type of site the mapping was bought for - root domain or sub domain, for a simple or AT site - there should be one recommended setup (usually setting name servers) and one or two alternative, more advanced, options, usually setting A or CNAME records.

Testing instructions

Both simple and atomic sites should be tested, as well as root domain mappings and subdomain mappings.

In Calypso, go to Upgrades > Domains > Add a domain to this site
Select "Use a domain I own"
"Map your domain"
Insert a root domain or subdomain
Buy that domain mapping for your site
The "Thank you" page with further instructions to setup domain mapping will be shown.

Root domain mappings instructions differ if the site is simple or atomic.

For simple sites: The recommended setup should be to set NS records pointing to our name servers, and the advanced setup should show the default IP addresses (*.24 and *.25) to be set as A records
For AT sites: The recommended setup should be to set NS records pointing to our name servers, and the advanced setup should show the AT site's IP addresses to be set as A records

Subdomain mappings should show the following instructions: The recommended setup should be to set CNAME or NS records pointing to our name servers, and no advanced setup should be shown

matticbot · 2021-03-12T13:39:02Z

Test live: https://calypso.live/?branch=update/domain-mapping-thank-you-page

matticbot · 2021-03-12T13:40:14Z

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

Sections (~1643 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
checkout      +3374 B  (+0.2%)    +1247 B  (+0.3%)
domains       +1531 B  (+0.1%)     +396 B  (+0.1%)

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.

klimeryk

Leaving comments from our call together so that they are not lost.
Sorry for the brevity - the code and approach is overall looking really good. Good job figuring out all the moving pieces from other places in our code!

klimeryk · 2021-03-15T17:44:25Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

-	) : (
-		<span />
-	);
+	const renderLinkTo = ( url ) => {


Let's use ExternalLink here.

klimeryk · 2021-03-15T17:44:32Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

+	const primaryMessage = translate(
+		'Please log into your account at your domain registrar and {{strong}}update the name servers{{/strong}} of your domain to use the following values, as detailed in {{link}}these instructions{{/link}}:',
+		{
+			comment: 'Notice for mapped domain notice with NS records pointing to somewhere else',


This might not be needed.

klimeryk · 2021-03-15T17:45:17Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

-					<li>{ domain }. IN NS ns1.wordpress.com.</li>
-					<li>{ domain }. IN NS ns2.wordpress.com.</li>
-					<li>{ domain }. IN NS ns3.wordpress.com.</li>
+	const isDataLoaded = () => {


Let's maybe refactor the whole component to be a normal one - or at least, pull these methods out of this component.

klimeryk · 2021-03-15T17:46:18Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

+	};
+
+	const renderARecordsList = () => {
+		if ( isDataLoaded() ) {


Maybe let's inverse the logic here - and return the placeholder component early. As that's the least interesting case (and avoids big nested lines).

klimeryk · 2021-03-15T17:47:34Z

client/my-sites/checkout/checkout-thank-you/style.scss

+		margin: 0;
+	}
+	.purchase-detail__image {
+		@include breakpoint-deprecated( '>660px' ) {


Let's try to use the new Gutenberg mixins in new code: https://github.com/Automattic/wp-calypso/blob/HEAD/docs/coding-guidelines/css.md#media-queries
And https://github.com/WordPress/gutenberg/blob/0f1f5e75408705f0ec014f5d2ea3d9fcc8a97817/packages/base-styles/_breakpoints.scss

klimeryk

Overall, I really like the separate component that we will be then be able to share in different places. Left a few comments from our call.

Be sure to also test with ?flag=force-sympathy in development mode to simulate loading Calypso with an empty Redux tree.

klimeryk · 2021-03-16T14:01:06Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

+		domain: PropTypes.object,
+		isAtomic: PropTypes.bool,
+		isLoaded: PropTypes.bool,
+		translate: PropTypes.func.isRequired,


By convention, translate is not included in the prop types, as it comes from the localize HOC wrapper.

klimeryk · 2021-03-16T14:02:30Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

+		isLoaded: false,
+	};
+
+	render() {


By convention, render is usually the last method in the component. The way I've always approached it was go from most internal to most "public" ones. So, lifecycle methods like willReceiveProps, etc. first and then smaller render* methods. And then finally the main render method.

klimeryk · 2021-03-16T14:10:11Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

+	};
+
+	render() {
+		const { domain } = this.props;


To avoid inconsistent messaging as the domain object is loaded, let's maybe simply show a placeholder here and return early.

klimeryk · 2021-03-16T14:23:17Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

+	static propTypes = {
+		domain: PropTypes.object,
+		isAtomic: PropTypes.bool,
+		isLoaded: PropTypes.bool,


Let's maybe put a more descriptive variable name here - it's unclear what exactly "is loaded" otherwise.

klimeryk · 2021-03-16T14:26:39Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

+
+class DomainMappingInstructions extends React.Component {
+	static propTypes = {
+		domain: PropTypes.object,


Instead of passing in the whole domain object, maybe it'd be better to pass in only the props we need:

domain name

A records list

areDomainDetailsLoaded type of prop that will probably look like ! isRequestingSiteDomains && domain - meaning, do we have the domain object and we're not waiting for a new version of it.

klimeryk · 2021-03-16T14:30:18Z

client/my-sites/domains/domain-management/domain-mapping-instructions/index.jsx

@@ -0,0 +1,143 @@
+/**


As discusses on call, maybe let's put it in my-sites/domains/components/mapping-instructions, since this will be shared between domain management and checkout 🙇

klimeryk · 2021-03-16T14:35:13Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

 		</div>
 	);
 };

 const mapStateToProps = ( state, { domain } ) => {
 	const selectedSite = getSelectedSite( state );
 	return {
+		domains: getDomainsBySiteId( state, selectedSite.ID ),
 		isBusinessPlan: isBusiness( selectedSite.plan ),


Probably need to add something like isRequestingSiteDomains: isRequestingSiteDomains( state, selectedSite.ID ) here from import { isRequestingSiteDomains } from 'calypso/state/sites/domains/selectors';.

klimeryk

Awesome! This looks almost perfect - I've just left a few nitpicks, but we're almost there ❤️

klimeryk · 2021-03-17T13:48:49Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

 	const selectedSite = getSelectedSite( state );
 	return {
+		domains: getDomainsBySiteId( state, selectedSite.ID ),


Suggested change

const selectedSite = getSelectedSite( state );

return {

domains: getDomainsBySiteId( state, selectedSite.ID ),

const selectedSite = getSelectedSite( state );

const domains = getDomainsBySiteId( state, selectedSite.ID );

return {

purchasedDomain: getSelectedDomain( { domains, selectedDomainName: domain } );

I'd move fetching the selected domain name to here, instead of doing it inside the component.

klimeryk · 2021-03-17T13:49:47Z

client/my-sites/checkout/checkout-thank-you/domain-mapping-details.jsx

 import { getSelectedSite } from 'calypso/state/ui/selectors';
+import QuerySiteDomains from 'calypso/components/data/query-site-domains';
+import { getDomainsBySiteId, isRequestingSiteDomains } from 'calypso/state/sites/domains/selectors';
+import DomainMappingInstructions from 'calypso/my-sites/domains/components/mapping-instructions';

 const DomainMappingDetails = ( {
 	domain,


Maybe to avoid confusion on whether domain is the domain object or string, you could rename it here like this:

Suggested change

domain,

domain: domainName,

klimeryk · 2021-03-17T13:58:39Z

client/components/purchase-detail/index.jsx

@@ -78,7 +78,7 @@ export default class PurchaseDetail extends PureComponent {

 		return (
 			<div className="purchase-detail__body">
-				{ this.renderPurchaseButton() }
+				{ this.props.href && this.renderPurchaseButton() }


This change might not be needed or correct in fact - I can imagine a use case where onClick is the only handler passed (like navigating to a different section in Calypso) and this change would break that behaviour.

Maybe we're misusing this component at all - instead of passing in description, it should not be a body?

leonardost · 2021-03-17T18:35:45Z

client/my-sites/domains/domain-management/edit/style.scss

-
-			> div:last-of-type .foldable-faq__answer {
-				border-bottom: 0;
-			}


This rule was conflicting with another one in mapping-instructions/style.scss

klimeryk

Tested again and all looks great! 💖

Update instrucitons to be the same as those defined in #50668. Only root domain mapping instructions were updated in this commit, and A records are still not being shown.

Moved from `my-sites/domains/domain-management/` to `my-sites/domains/components/` as the component will be shared between doman management and checkout.

…pping-instructions`

…tons`

… already fetched

… details are not loaded

…l screens

The removed check was redundant.

…Instructions component

Continuing the changes done in #51031 and #50668, this PR updates the subdomain mapping instructions that are presented to users. These instructions are shown in the thank you page, after the user buys the subdomain mapping, and in the domains management page, when a subdomain isn't mapped yet. This PR presents CNAME or NS records mappings as recommended setups depending if the site is simple or AT, respectively. Alternative mapping setups (e.g. using A records) are shown as advanced setup options.

a8ci18n · 2021-03-31T20:20:32Z

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/5639196

Hi @leonardost, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include this string: Please set the following IP addresses as A records for %(subdomain)s using {{link}}these instructions{{/link}}:

Thank you in advance!

a8ci18n · 2021-04-08T16:28:30Z

Translation for this Pull Request has now been finished.

leonardost added [Feature Group] Emails & Domains Features related to email integrations and domain management. [Status] In Progress Back to Basics labels Mar 12, 2021

klimeryk reviewed Mar 15, 2021

View reviewed changes

klimeryk reviewed Mar 16, 2021

View reviewed changes

leonardost force-pushed the update/domain-mapping-thank-you-page branch from 009ebf2 to 5a86e40 Compare March 16, 2021 17:48

leonardost marked this pull request as ready for review March 17, 2021 13:37

leonardost requested a review from a team as a code owner March 17, 2021 13:37

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 17, 2021

leonardost removed the [Status] In Progress label Mar 17, 2021

klimeryk reviewed Mar 17, 2021

View reviewed changes

leonardost commented Mar 17, 2021

View reviewed changes

klimeryk approved these changes Mar 18, 2021

View reviewed changes

leonardost added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Mar 18, 2021

leonardost added 13 commits March 18, 2021 15:45

Update PurchaseDetail to show button only if a href is given

68a1827

Update domain mapping thank you page instructions

154026a

Update instrucitons to be the same as those defined in #50668. Only root domain mapping instructions were updated in this commit, and A records are still not being shown.

Show A records in domain mapping thank you page instructions

6fb7765

Change a tag for ExternalLink component

e3f01fa

Remove redundant comment from translation string

d9499ab

Invert conditions in renderARecordsList method to improve readability

ba4b7ec

Remove unnecesary CSS rule

a2ae8ae

Replace deprecated mixin for break-mobile

873b1d9

Extract some functions from DomainMappingDetails component

6be450d

Add subdomain mapping instrucitons to mappings thank you page

6598d5f

Extract mapping instructions into DomainMappingInstructions component

4c50d2b

Move CSS rule from DomainMappingDetails to DomainMappingInstructions

b1986c9

Remove DomainMappingInstructions code from DomainMappingDetails

635e9b7

leonardost added 17 commits March 18, 2021 15:45

Remove unneeded translate from PropTypes

cba6112

Invert order of render* methods to adhere to convention

b042cfd

Change location of DomainMappingInstructions component

d2a2b20

Moved from `my-sites/domains/domain-management/` to `my-sites/domains/components/` as the component will be shared between doman management and checkout.

Alter component folder name from domain-mapping-instructions to `ma…

8653328

…pping-instructions`

Rename attribute isLoaded to areDomainDetailsLoaded

19184a9

Pass aRecords and domainName properties to `DomainMappingInstruci…

ffb092e

…tons`

Update areDomainDetailsLoaded logic to check if domain details were…

2ae5c95

… already fetched

Show placeholder for DomainMappingInstructions component while domain…

af0f6b7

… details are not loaded

Reduce padding around mapping instructions in thank you page for smal…

19faff9

…l screens

Add padding around mapping instrucitons placeholder

f470832

Move purchasedDomain attribute inside mapStateToProps

9272cbf

Rename domain property to selectedDomainName

6fd2b3d

Revert unnecessary check in PurchaseDetail to hide button

d4226e6

The removed check was redundant.

Use body attribute of PurchaseDetail instead of description

5783fb6

Fix identation of DomainMappingInstructions' style.scss file

f708afe

Replace mapping instructions in MappedDomainType by the DomainMapping…

baf8857

…Instructions component

Remove conflicting CSS rule from domain-management/edit/style.scss

9808de3

leonardost force-pushed the update/domain-mapping-thank-you-page branch from 13b0a98 to 9808de3 Compare March 18, 2021 18:47

leonardost merged commit 381247b into trunk Mar 18, 2021

leonardost deleted the update/domain-mapping-thank-you-page branch March 18, 2021 19:14

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

leonardost mentioned this pull request Mar 19, 2021

Domains: Update instructions for mapping subdomains #51251

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Domains: Update domain mapping thank you page #51031

Domains: Update domain mapping thank you page #51031

leonardost commented Mar 12, 2021 •

edited

matticbot commented Mar 12, 2021

matticbot commented Mar 12, 2021 •

edited

klimeryk left a comment

klimeryk Mar 15, 2021

klimeryk Mar 15, 2021

klimeryk Mar 15, 2021

klimeryk Mar 15, 2021

klimeryk Mar 15, 2021

klimeryk left a comment

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk Mar 16, 2021

klimeryk left a comment

klimeryk Mar 17, 2021

klimeryk Mar 17, 2021

klimeryk Mar 17, 2021

leonardost Mar 17, 2021

klimeryk left a comment

a8ci18n commented Mar 31, 2021

a8ci18n commented Apr 8, 2021

Domains: Update domain mapping thank you page #51031

Domains: Update domain mapping thank you page #51031

Conversation

leonardost commented Mar 12, 2021 • edited

Changes proposed in this Pull Request

Testing instructions

matticbot commented Mar 12, 2021

matticbot commented Mar 12, 2021 • edited

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimeryk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klimeryk left a comment

Choose a reason for hiding this comment

a8ci18n commented Mar 31, 2021

a8ci18n commented Apr 8, 2021

leonardost commented Mar 12, 2021 •

edited

matticbot commented Mar 12, 2021 •

edited