-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Domain Upsell Back Button: Previous Page or Preferred View #91957
Conversation
Jetpack Cloud live (direct link)
Automattic for Agencies live (direct link)
|
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: Sections (~40 bytes added 📈 [gzipped])
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. Generated by performance advisor bot at iscalypsofastyet.com. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some drive by comments 🚗
I was only able to find the Free domain with an annual plan
upsell for a site on the free plan. For a new site on an annual Creator plan without a domain, I get a different upsell (Free domain available
). Do you know when this upsell is activated, @chad1008?
if ( window.history.length > 1 ) { | ||
window.history.go( -1 ); | ||
return; | ||
} | ||
|
||
if ( props.goBackLink ) { | ||
window.location.assign( props.goBackLink ); | ||
} else { | ||
window.history.go( -1 ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change seems to work, but history.length
isn't reduced after history.go( -1 )
, which means that if a user reaches the Select a plan
step, we'll always run this code instead of location.assign()
.
Maybe it's safer to revert this specific change..?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm comfortable with that. The motivation for this change was the
It should link to the previous page if possible. If not, go to
:site/wp-admin
if the site is configured to the classic interface. Otherwise, go to/home/:site
portion of the related issue's acceptance criteria, but it did feel a little odd to me that we'd virtually never, or just never never, actually make it to the location.assign
flow.
If we revert this change, I'd probably also opt to revert the change to the button text, and make it say Home by default, and conditionally say Back if that path is utilized (which is what it does currently on production)
Effectively, this PR would really just make sure the Home path is conditional based on the user's interface preference, but leave all other behavior as it is.
That all works for me. Let me know if you have any objections/concerns @arthur791004 🙂
@@ -444,6 +449,8 @@ export default connect( | |||
isSiteOnWooExpress( state, siteId ) || | |||
isSiteOnEcommerce( state, siteId ), | |||
isFromMyHome: getCurrentQueryArguments( state )?.from === 'my-home', | |||
preferredView: getPreferredEditorView( state, siteId ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getPreferredEditorView
seems to only be used in the context of the site editor. I don't know that it's wrong to use here, but I want to double check if you've looked at other candidates, @chad1008?
What we're looking for here is the value of this setting, right..? cc @arthur791004
I guess we might be introducing a similar setting for Simple sites with this project pfsHM7-EF-p2
![Screenshot 2024-06-26 at 12 42 17](https://private-user-images.githubusercontent.com/1101677/343123156-ce0ea3bd-4399-48ff-b350-813117c72263.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE1MDI5MzQsIm5iZiI6MTcyMTUwMjYzNCwicGF0aCI6Ii8xMTAxNjc3LzM0MzEyMzE1Ni1jZTBlYTNiZC00Mzk5LTQ4ZmYtYjM1MC04MTMxMTdjNzIyNjMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDcyMCUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA3MjBUMTkxMDM0WiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9MTlmNGQ3NGE4ZTYzNjUwODBiOGY4ODM5YzliMDFjNTRjYzNkMmE2NjU2MTU4NWE4YjQxZGM2OTZhMGM1NGIzMCZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.yKnYhmQIiYegeon4MXziPt6xcp26VBQ-67uDWI4Ijiw)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value can be retrieved by getSiteOption( state, site?.ID, 'wpcom_admin_interface' );
- Classic style - the value equals to
wp-admin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @fredrikekelund and @arthur791004. I'll update to PR to use this this method instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up on this feedback, I think I may have started work on a free site, and then for whatever reason completed an upgrade, which gave me access to the Settings > General > Admin Interface that isn't normally available on free sites.
As @fredrikekelund noted above, that option is coming for simple sites, so I've added the wpsh
command to enable that setting to the testing instructions above.
The upsell we're working on (Free domain with an annual plan) is only triggered for free sites (you're correct, there. I've updated my testing instructions to be more clear on that front!). The other upsell (Free domain available) appears to by triggered by any site with an available domain credit, but it goes to a different flow that this PR won't touch. |
Update: I've addressed the |
I've updated the PR to reflect the above conversation:
I've also updated the pr description and testing instructions to match these changes. @nightnei, would you mind looking this one over and sharing any thoughts/approving? We've shifted a bit from the original plan laid out by @arthur791004, so I'd love some extra eyes before we merge 😄 |
6742801
to
2c12be1
Compare
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
return; | ||
} | ||
|
||
if ( window.history.length > 1 ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You added the condition; hence, does it mean that we have a case when history length is <= 1? And in this case clicking on the button do nothing. We should definitely handle default behaviour. Or if it's not possible to have length less than 1, then let's remove the condition, since it's redundant and can just confuse developers in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, as I see, window.history.go( -1 );
was redundant before, since we always had goBackLink
, according to the old condition:
const goBackButtonProps = launchpadScreen === 'full'
? {
goBackText: translate( 'Next Steps' ),
goBackLink: `/setup/${ siteIntent }/launchpad?siteSlug=${ selectedSiteSlug }`,
}
: {
goBackLink: `/home/${ selectedSiteSlug }`,
};
So, maybe we can just remove it?
UPDATE: now you added this.props.preferredView === 'wp-admin
, so looks like, only now, it can be undefined. But can it be?
AFAIU, if there is this.props.preferredView
, then we should definitely have getSiteOption( state, siteId, 'admin_url' )
from getSiteAdminUrl
, or I understand incorrectly?
Could you please check it, since I suppose that we can just delete it. And avoid having code just to have code which does nothing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these points @nightnei. You're right that with the original logic, the window.history.go(-1)
was redundant, because goBackLink
was always populated by a string. As long as that holds true, after addressing the rest of your feedback, I agree it can be removed.
Looking at the new logic, you're also right that wpAdminUrl
could be null
. I think the current change is still mostly safe, because (as you pointed out) if we're able to declare preferredView
as wp-admin
, then we should have no trouble retrieving the wpAdminUrl
. Both of those selectors are relying on siteId
, so if one succeeds, they should both succeed. If preferredView
WAS null
, we'd skip to the second path of the ternary, and the having wpAdminUrl
be null
doesn't matter. We'd be sending them to Calypso at that point.
But I hate that I used the word "should" just now. I don't see a scenario where preferredView === 'wp-admin'
but wpAdminUrl === null
, but better safe than sorry. I'm going to add a check of the wpAdminUrl
value into that ternary so we can be sure it's populated before using for the goBackLink
value.
client/my-sites/domains/components/domain-and-plan-package/navigation/index.jsx
Outdated
Show resolved
Hide resolved
@@ -323,9 +324,11 @@ class DomainSearch extends Component { | |||
goBackLink: `/setup/${ siteIntent }/launchpad?siteSlug=${ selectedSiteSlug }`, | |||
} | |||
: { | |||
goBackLink: `/home/${ selectedSiteSlug }`, | |||
goBackLink: | |||
this.props.preferredView === 'wp-admin' && !! this.props.wpAdminUrl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 👍
@@ -6,11 +6,12 @@ import './style.scss'; | |||
export default function DomainAndPlanPackageNavigation( props ) { | |||
const translate = useTranslate(); | |||
|
|||
// `goBackLink` will be either wp-admin or My Home, depending on the user's | |||
// current admin interface preference. | |||
// If unavailable, we fall back to the previous page in history. | |||
const goBack = () => { | |||
if ( props.goBackLink ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it seems, now, we can remove this condition too
if ( props.goBackLink ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we expect goBackLink
to always be defined in the props, then we should enforce it. I suggest we do that by converting this file to TS 👍
@@ -26,7 +29,7 @@ export default function DomainAndPlanPackageNavigation( props ) { | |||
return ( | |||
<div className="domain-and-plan-package-navigation"> | |||
<div className="domain-and-plan-package-navigation__back"> | |||
<Button borderless="true" onClick={ goBack }> | |||
<Button borderless onClick={ goBack }> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After converting the file, TS flagged this boolean-as-string. After fixing, the linter helpfully pointed out that the value should be omitted. I've tested locally to confirm the button style isn't impacted by this change.
Worked through most of the TS conversion work before I ran out of time tonight. Still a WIP, as there are a couple of pesky errors left, and It likely needs some refinement. I got a little heavy handed in some points and there are probably more elegant solutions to some of these errors. Feedback is welcome if either of you feel like taking a look @fredrikekelund/@nightnei |
168694d
to
45a9abf
Compare
} ); | ||
this.isMounted && page( domainMappingUrl ); | ||
handleAddMapping = ( domain: string ) => { | ||
if ( this.props.selectedSiteSlug ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check ensures selectedSiteSlug
is not null
before proceeding. I'm not thrilled with this approach, and would love some feedback. I'd like to be able to more confidently assert that it will never be null
(rather than just appeasing TS as I'm doing now), or add something to ensure a null
value is handled gracefully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see :(
Let's go the next way:
// Just TS typing fix, we always have selectedSiteSlug
if ( !this.props.selectedSiteSlug ) {
return;
}
.... rest of teh function
Cooment will be very useful, in case if in the future somebody does refactoring. Also it will help to understand the purpose, if another Code Wrangler will do maintenance of this function
try { | ||
await this.props.shoppingCartManager.addProductsToCart( [ domainTransfer( { domain } ) ] ); | ||
await this.props.shoppingCartManager.addProductsToCart( [ | ||
domainTransfer( { domain, extra: {} } ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This empty object works, but I don't really like it.
domainTransfer
requires an extra
prop, but all of the properties of that interface are optional. I'm tempted to make the extra
param of this function optional itself, rather than being forced to pass this empty object. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I belive that we can safely update domainTransfer
function, by setting extra
as optional value. It should not make any issues in the code. And additionally the function expects extra as optional, since inside we have ternary operator ...( properties.extra ? { extra: properties.extra } : {} ),
, so let's proceed with updating domainTransfer
function and remove this empty object {}
.
@@ -173,7 +204,7 @@ class DomainSearch extends Component { | |||
|
|||
let registration = domainRegistration( { | |||
domain, | |||
productSlug, | |||
productSlug: productSlug as string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dislike this immensely, but drew a blank a better approach than just heavy-handedly sidestepping the potential null
value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :(
@@ -257,13 +288,14 @@ class DomainSearch extends Component { | |||
return query.redirect_to; | |||
} | |||
|
|||
return domainManagementList( selectedSiteSlug, currentRoute ); | |||
return domainManagementList( selectedSiteSlug ?? undefined, currentRoute ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treating a potential null
value as undefined
to match the shape of domainManagementList
const site = getSelectedSite( state ); | ||
const siteId = getSelectedSiteId( state ); | ||
const siteId = getSelectedSiteId( state ) ?? undefined; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Treating potential null
value as undefined
to conform to the shape of the selectors we're calling below.
cc @nightnei Okay, typing Added inline comments for a couple of spots where I specifically want some advice, but feedback on everything is very welcome. |
isEcommerceSite: siteId | ||
? isSiteOnECommerceTrial( state, siteId ) || | ||
isSiteOnWooExpress( state, siteId ) || | ||
isSiteOnEcommerce( state, siteId ) | ||
: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can simplify it w/o hardcodding false
:
isEcommerceSite: siteId | |
? isSiteOnECommerceTrial( state, siteId ) || | |
isSiteOnWooExpress( state, siteId ) || | |
isSiteOnEcommerce( state, siteId ) | |
: false, | |
isEcommerceSite: !! siteId && ( | |
isSiteOnECommerceTrial( state, siteId ) || | |
isSiteOnWooExpress( state, siteId ) || | |
isSiteOnEcommerce( state, siteId ) | |
), |
isSiteOnECommerceTrial( state, siteId ) || | ||
isSiteOnWooExpress( state, siteId ) || | ||
isSiteOnEcommerce( state, siteId ), | ||
isSiteOnFreePlan: !! site && site.plan && isFreePlanProduct( site.plan ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we convert site to boolean, hence let's do it for site.plan
, to have consistency:
isSiteOnFreePlan: !! site && site.plan && isFreePlanProduct( site.plan ), | |
isSiteOnFreePlan: !! site && !! site.plan && isFreePlanProduct( site.plan ), |
@@ -435,20 +469,23 @@ export default connect( | |||
selectedSiteId: siteId, | |||
selectedSiteSlug: getSelectedSiteSlug( state ), | |||
domainsWithPlansOnly: currentUserHasFlag( state, DOMAINS_WITH_PLANS_ONLY ), | |||
isSiteUpgradeable: isSiteUpgradeable( state, siteId ), | |||
isSiteOnMonthlyPlan: isSiteOnMonthlyPlan( state, siteId ), | |||
isSiteUpgradeable: !! siteId && isSiteUpgradeable( state, siteId ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just thought out loud - I don't like this extra condition, but unfortunately it's indeed the best way, to avoid rewriting all functions like isSiteUpgradeable
:(
@@ -173,7 +204,7 @@ class DomainSearch extends Component { | |||
|
|||
let registration = domainRegistration( { | |||
domain, | |||
productSlug, | |||
productSlug: productSlug as string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep :(
try { | ||
await this.props.shoppingCartManager.addProductsToCart( [ domainTransfer( { domain } ) ] ); | ||
await this.props.shoppingCartManager.addProductsToCart( [ | ||
domainTransfer( { domain, extra: {} } ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I belive that we can safely update domainTransfer
function, by setting extra
as optional value. It should not make any issues in the code. And additionally the function expects extra as optional, since inside we have ternary operator ...( properties.extra ? { extra: properties.extra } : {} ),
, so let's proceed with updating domainTransfer
function and remove this empty object {}
.
} ); | ||
this.isMounted && page( domainMappingUrl ); | ||
handleAddMapping = ( domain: string ) => { | ||
if ( this.props.selectedSiteSlug ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see :(
Let's go the next way:
// Just TS typing fix, we always have selectedSiteSlug
if ( !this.props.selectedSiteSlug ) {
return;
}
.... rest of teh function
Cooment will be very useful, in case if in the future somebody does refactoring. Also it will help to understand the purpose, if another Code Wrangler will do maintenance of this function
thanks for the feedback and advice @nightnei. All feedback should be addressed, and hopefully good to go |
…igation/index.jsx Co-authored-by: Volodymyr Makukha <nei.css@gmail.com>
simplify/clean up declarations Co-authored-by: Volodymyr Makukha <nei.css@gmail.com>
cb8794f
to
3f77290
Compare
7639-gh-Automattic/dotcom-forge
Proposed Changes
Have the "Home" link on the domain with plan upsell page take you to either wp-admin or My Home, depending on your current site admin interface preferences. If this value isn't available, we'll keep using the most recent page from browser history as a backup.
The issue linked above recommends using browser history first and My Home/wp-admin as the fallback, but we've shifted away from that for the reasons discussed in the comments.
Why are these changes being made?
Sending a site owner who prefers wp-admin to My Home could be jarring, and not the best experience, since they've expressed that this isn't how the prefer to interact with their site.
Testing Instructions
wpsh
update_blog_option( blog_id, 'wpcom_admin_interface', 'wp-admin' );
update_blog_option( blog_id, 'wpcom_classic_early_release', 1 );
wpsh
on your sandbox:update_blog_option( blog_id, 'wpcom_admin_interface', 'default' );
10. To simulate a scenario where our desired "home" link isn't available, apply the following diff: