-
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
Generalize marketplace thank you content #54577
Conversation
4af6e81
to
4d71fd8
Compare
Link to Calypso live: https://calypso.live?image=registry.a8c.com/calypso/app:build-11722 |
}, | ||
{ | ||
stepKey: 'titan_whats_next_manage_email', | ||
stepTitle: translate( 'Manage your email' ), |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 25 times:
translate( 'Manage your email settings' )
ES Score: 6
See 2 additional suggestions in the PR translation status page
Hi @daledupreez I never imagined the new designs for the marketplace thank you page will start to be used in other areas so this component will be incredibly hard to refactor in a maintainable way without leaving confusing remnants. This start is great and will be more reusable for other variations of a thank you page. However, I personally think this approach of a newer generic version of a thank you page should be written separately from scratch. If anything feel free to move and/or duplicate, the already available styled UI components used inside the Yoast thank you page in a new generic location, so that it can be imported back into the Yoast. Basically just move all the code you require for the generic implementation of this thank you page into a seperate, suitable location with appropriate naming ( Eg: Other than that I think you are on the right path! :) |
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~85 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~3253 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. |
Thanks for the comments, @jdc91! I agree that we should move the Thank You content out more completely, but I wanted this PR to explore the general shape of the refactor and approach. I'll do more to separate out the pieces over the next few days, and I think that should help this move towards truly general components. |
dffa139
to
7e753e9
Compare
A heads-up that I've rebased that pull request off |
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 left quite a few comments. I've also noticed a very subtle issue with scrolling - when I scroll up or down I get block mid-course:
It feels like we have an invisible scrollbar for some elements of the page - so I start scrolling in that element and when I reach its end the page itself finally scrolls (see how the scrollbar behaves).
const translate = useTranslate(); | ||
const thankYouImageSrc = '/calypso/images/upgrades/thank-you.svg'; | ||
|
||
const { domainName, emailAddress = `youremail@${ domainName }` } = props; |
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 not a big fan of showing youremail@
when the email address is not available, as it's not localized. We could simply show Your email is now ready to use
in the header of the page when that's the case.
stepKey: 'titan_whats_next_get_mobile_app', | ||
stepTitle: translate( 'Get mobile app' ), | ||
stepDescription: translate( | ||
"Access your email on the go with Titan's Android and iOS apps." |
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 was about to mention that we should refer to Professional Email
instead of Titan
but after giving it more thoughts this is probably one of the few places where Titan
is a better option - users will download a Titan
app after all. However if we keep it here, we should probably add it on the Manage All Mailboxes
page then:
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 string in the Manage All Mailboxes
page has not been updated, it should be:
Download Titan's Android and iOS apps to access your emails on the go
This PR modifies the release build for wpcom-block-editor To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-l4k-p2 |
This PR modifies the release build for notifications To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-elI-p2 |
This PR modifies the release build for o2-blocks To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-r7r-p2 |
This PR modifies the release build for editing-toolkit To test your changes on WordPress.com, run To deploy your changes after merging, see the documentation: PCYsg-mMA-p2 |
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 of my previous feedback has not been addressed but this concerns minor changes. I've made some proposals regarding the route of the new Thank You
page for Titan but we can wait for #54934 to be deployed before refining that. There is one blocker though, which is the display of these pages for Titan and Yoast which have either the top or the bottom cut:
} | ||
`; | ||
|
||
const MarketplaceNextSteps = styled.div< MarketplaceThemeProps >` |
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.
Shoudln't we rename MarketplaceNextSteps
to ThankYouNextSteps
?
*/ | ||
import { CALYPSO_CONTACT, SUPPORT_ROOT } from 'calypso/lib/url/support'; | ||
import { MarketplaceThemeProps } from 'calypso/my-sites/marketplace/theme'; | ||
import { MarketplaceHeaderTitle } from 'calypso/my-sites/marketplace/components'; |
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 are still referencing files from calypso/my-sites/marketplace
, it would be great to find a way to address that. That would be for another pull request though.
pageContext.primary = ( | ||
<TitanSetupThankYou | ||
domainName={ pageContext.params.domain } | ||
emailAddress={ pageContext.query.email } |
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 TitanSetupThankYou
component is currently designed to retrieve the email address as a query parameter:
https://wordpress.com/email/:domain/titan/thank-you/:site?email=:email
Since :email
should always point to the same domain than :domain
, what if asked for the mailbox instead?
https://wordpress.com/email/:domain/titan/thank-you/:site?mailbox=:mailbox
We can indeed build the email address from these two pieces of information inside the TitanSetupThankYou
component (i.e. :mailbox@:domain
). We could even add that mailbox to the path so the url is even shorter:
https://wordpress.com/email/:domain/titan/thank-you/:mailbox/:site
That would give us something like:
https://wordpress.com/email/example.com/titan/thank-you/contact/example.wordpress.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.
Thinking about it more, since we will introduce the new page for setting up the unused mailbox at:
https://wordpress.com/email/:domain/titan/setup/:site
Or at - depending on the outcome of this discussion:
https://wordpress.com/email/:domain/titan/setup-mailbox/:site
It probably makes sense to move this new Thank You
page at:
https://wordpress.com/email/:domain/titan/setup/thank-you/:mailbox/:site
* to the store that is relative to hum (Windows Store? Apple Store? Android Store? Web app?*/ | ||
stepCta: ( | ||
<FullWidthButton href="https://titan.email"> | ||
{ translate( 'Get the app' ) } |
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.
Looks like this also felt through the cracks.
stepKey: 'titan_whats_next_get_mobile_app', | ||
stepTitle: translate( 'Get mobile app' ), | ||
stepDescription: translate( | ||
"Access your email on the go with Titan's Android and iOS apps." |
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 string in the Manage All Mailboxes
page has not been updated, it should be:
Download Titan's Android and iOS apps to access your emails on the go
@@ -134,7 +134,7 @@ class TitanManageMailboxes extends Component { | |||
|
|||
<VerticalNavItemEnhanced | |||
description={ translate( | |||
'Download our Android and iOS apps to access your emails on the go' | |||
"Download Titan's Android and iOS apps to access your emails on the go" |
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 will be queued for retranslation. We'll use the existing translations in the meantime.
<FullWidthButton | ||
href={ getControlPanelUrl( TITAN_CONTROL_PANEL_CONTEXT_GET_MOBILE_APP ) } | ||
> | ||
{ translate( 'Get app' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 66 times:
translate( 'Get Apps' )
ES Score: 7
@@ -134,7 +134,7 @@ class TitanManageMailboxes extends Component { | |||
|
|||
<VerticalNavItemEnhanced | |||
description={ translate( | |||
'Download our Android and iOS apps to access your emails on the go' | |||
"Download Titan's Android and iOS apps to access your emails on the go" |
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 will be queued for retranslation. We'll use the existing translations in the meantime.
<FullWidthButton | ||
href={ getControlPanelUrl( TITAN_CONTROL_PANEL_CONTEXT_GET_MOBILE_APP ) } | ||
> | ||
{ translate( 'Get app' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 66 times:
translate( 'Get Apps' )
ES Score: 7
* Fix scroll/height issues * Fix top margin/padding CSS, including mobile fixes * Use default CSS for header image width * Re-add custom Masterbar for Yoast marketplace
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 have some tweaks stemming from inspection, but I had continued scroll/cut-off content issues when testing, and those don't appear when accessing the existing Yoast thank you page via wpcalypso.
So I dived in and tried to clean up the CSS and some related things I noticed. There are still some smaller items, but I think I simplified the really nasty scroll and scroll-bar issues that were making the component hard to work with, and I noticed a subtle issue where we'd affect the CSS for .layout_content
across all pages!
I'll push up my fixes shortly, but I think we can wrap this up with a pairing session in the morning.
/** | ||
* External dependencies | ||
*/ | ||
import { TranslateResult, useTranslate } from 'i18n-calypso'; |
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.
Minor note: I think we can split this up so we're only importing the type for TranslateResult
- I believe that means we only use the import during compilation and not at runtime.
import { TranslateResult, useTranslate } from 'i18n-calypso'; | |
import type { TranslateResult } from 'i18n-calypso'; | |
import { useTranslate } from 'i18n-calypso'; |
'Download our Android and iOS apps to access your emails on the go' | ||
"Download Titan's Android and iOS apps to access your emails on the go" |
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.
Can you move this to a separate PR? While we noticed it here, I don't think it's directly tied to the Thank You content.
client/my-sites/email/paths.js
Outdated
return emailManagementEdit( | ||
siteName, | ||
domainName, | ||
'titan/thank-you', |
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 agree with @stephanethomas's suggestion that we add this under titan/setup/thank-you
instead.
'titan/thank-you', | |
'titan/setup/thank-you', |
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.
Actually, I take that back! Let's start with titan/setup-mailbox/thank-you
to start.
'titan/thank-you', | |
'titan/setup-mailbox/thank-you', |
/** | ||
* Internal dependencies | ||
*/ | ||
import { ThankYou } from 'calypso/components/thank-you'; |
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.
Nit: these imports could be in alphabetical order. (Definitely not blocking.)
const getControlPanelUrl = ( context: string ) => { | ||
return emailManagementTitanControlPanelRedirect( selectedSiteSlug, domainName, currentRoute, { | ||
context, | ||
} ); | ||
}; |
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.
Nit: It looks like we're only using this function from one location. So why not just set the redirect URL directly into a constant?
const getControlPanelUrl = ( context: string ) => { | |
return emailManagementTitanControlPanelRedirect( selectedSiteSlug, domainName, currentRoute, { | |
context, | |
} ); | |
}; | |
const titanControlPanelUrl = emailManagementTitanControlPanelRedirect( | |
selectedSiteSlug, | |
domainName, | |
currentRoute, { | |
context: TITAN_CONTROL_PANEL_CONTEXT_GET_MOBILE_APP, | |
} | |
); |
sections={ [ titanThankYouSection ] } | ||
showSupportSection={ true } | ||
thankYouImage={ thankYouImage } | ||
thankYouTitle={ translate( 'Your email is now ready to use' ) } |
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.
It may be worth adding the email address here in a follow-up PR. We can either show "Your email is now ready to use"
when we don't have the email address, or show something more specific when we do have the email address. But let's take that on in a follow-up so we can get this PR shipped.
|
||
type TitanSetupThankYouProps = { | ||
domainName: string; | ||
emailAddress: 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 think we may want to make emailAddress
nullable/optional - I think this formulation makes it required.
@@ -134,7 +134,7 @@ class TitanManageMailboxes extends Component { | |||
|
|||
<VerticalNavItemEnhanced | |||
description={ translate( | |||
'Download our Android and iOS apps to access your emails on the go' | |||
"Download Titan's Android and iOS apps to access your emails on the go" |
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 will be queued for retranslation. We'll use the existing translations in the meantime.
<FullWidthButton | ||
href={ getControlPanelUrl( TITAN_CONTROL_PANEL_CONTEXT_GET_MOBILE_APP ) } | ||
> | ||
{ translate( 'Get app' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 66 times:
translate( 'Get Apps' )
ES Score: 7
- Alphabetic order of imports - Translation rollback for a different component - Refactored function to get Titan mailbox url - Changed URL to get the Thank you page
), | ||
stepCta: ( | ||
<FullWidthButton href={ titanControlPanelUrl }> | ||
{ translate( 'Get app' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 66 times:
translate( 'Get Apps' )
ES Score: 7
), | ||
stepCta: ( | ||
<FullWidthButton href={ titanControlPanelUrl } target="_blank"> | ||
{ translate( 'Get app' ) } |
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.
ℹ️ String reuse speeds up translation and improves consistency. The following string might make a good alternative and has already been translated 66 times:
translate( 'Get Apps' )
ES Score: 7
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.
Other than the small link issue I fixed in 36a3440, this LGTM!
Can you just double-check my changes before we deploy this? (I also created this, so I can't approve the PR!)
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/6305206 Hi @daledupreez, could you please edit the description of this PR and add a screenshot for our translators? Ideally it'd include all of the following strings:
Thank you in advance! |
Translation for this Pull Request has now been finished. |
Changes proposed in this Pull Request
MarketplaceThankYou
component added in Implement marketplace yoast checkout thank you page #52655 such that the same file exposes a more generalThankYou
component that supports the original use case as well as a new page we want to build for Professional Email (aka Titan).Testing instructions
There are two things we need to test:
For (1), I would love to get some confirmation from @jdc91 that the testing approach from #52655 is still valid. I kicked the tires here by navigating to
/marketplace/thank-you/:siteSlug
for an Atomic site where I manually installed the Yoast plugin, and that seemed to work, but I have no idea if that's valid!For (2), the visual work is incomplete for now, as I used the same image and didn't wire in all the relevant checks yet, but you can take a look at the visuals by accessing
/email/:domain/titan/thank-you/:siteSlug
on a site with a domain.