-
Notifications
You must be signed in to change notification settings - Fork 2k
New purchase cancelation dialog #67090
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
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~779 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~2817 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. Async-loaded Components (~781 bytes added 📈 [gzipped])
React components that are loaded lazily, when a certain part of UI is displayed for the first time. 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.
Approach looks good and I left some comments. Also add a screenshot if possible 👍
@@ -360,6 +388,7 @@ class RemovePurchase extends Component { | |||
<Gridicon className="card__link-indicator" icon="trash" /> | |||
</Wrapper> | |||
{ this.shouldShowNonPrimaryDomainWarning() && this.renderNonPrimaryDomainWarningDialog() } | |||
{ this.shouldShowPlanWarning() && this.renderPlanWarningDialog() } |
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 do not normally with this pattern of multiple render functions inside a component, but that seems to be the current approach of this file.
Personally I like to see a declarative component tree with lot's of ternaries and conditional logic which is much more readable at a glance. But YMMV.
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 was fixed
|
||
import './style.scss'; | ||
|
||
class RemovePlanDialog extends Component { |
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.
IMO it's better to use function components instead of class components. For one it's less code/boilerplate all round and its much simpler to read.
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.
.non-primary-domain-dialog { | ||
max-width: 500px; | ||
} |
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.
There is a prop called isFullScreen maybe you can utilize that prop here.
isFullScreen?: boolean; |
Also the guide to style the dialog component can be found in the docs.
https://wpcalypso.wordpress.com/devdocs/packages/components/src/dialog/README.md?term=Dialog
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/7513450 Thank you @ciprianimike for including a screenshot in the description! This is really helpful for our translators. |
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 sure how far along this is, but I tried it and the main thing I noticed is that I couldn't trigger it in the way I expected to:
- Purchased a plan
- Visited the purchase management page
- Clicked "Cancel Subscription and Refund" to remove the plan
- Didn't see the dialog anywhere during the process of removing the plan
In order to see it, I needed a plan that was no longer refundable and that already had auto-renew disabled (in other words, was already "cancelled" in some sense). Then I saw the dialog when I went to completely remove the subscription afterwards. Is that expected?
Also noticed that when I tried to remove a Pro plan, the feature list is missing:
I assume we should add a feature list for Pro and Starter (since there are plenty of those around that people might cancel), but either way, there should also be a fallback where if an unrecognized plan is being cancelled, the "If you cancel your plan, you will lose" sentence doesn't display.
const freeDomainFirstYear = String( translate( 'Your free domain for the first year' ) ); | ||
const freeDomain = String( | ||
translate( 'Your free domain www.%(slug)s', { | ||
args: { slug: domainSlug }, |
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 don't really understand the above wording.... We don't take away anyone's domain when they cancel the plan. We might subtract its cost from their refund (if the plan is within the refund window and the domain isn't) but that would be in the refund flow, which is different than the main flow you're in here.
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.
domains labels and logic were removed in the design vTj2aknM6tRUQndWtOcURT-fi-445%3A10488
} ) | ||
); | ||
const bestInClassHosting = String( translate( 'Best-in-class hosting' ) ); | ||
const addFreeSite = String( translate( 'An add-free site' ) ); |
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.
Should be "ad-free"
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 was fixed, the copy structure refatore, see
I'm not sure how far along this is, but I tried it and the main thing I noticed is that I couldn't trigger it in the way I expected to:
- Purchased a plan
- Visited the purchase management page
- Clicked "Cancel Subscription and Refund" to remove the plan
- Didn't see the dialog anywhere during the process of removing the plan
In order to see it, I needed a plan that was no longer refundable and that already had auto-renew disabled (in other words, was already "cancelled" in some sense). Then I saw the dialog when I went to completely remove the subscription afterwards. Is that expected?
Also noticed that when I tried to remove a Pro plan, the feature list is missing:
I assume we should add a feature list for Pro and Starter (since there are plenty of those around that people might cancel), but either way, there should also be a fallback where if an unrecognized plan is being cancelled, the "If you cancel your plan, you will lose" sentence doesn't display.
The logic changed, the modal now support subscription and you should be able to access the modal consistently.
PRO and Starte plans were added.
Copy is updated and moved to https://github.com/Automattic/wp-calypso/pull/67090/files#diff-8f9fb14a44111dd55e67d9462573bf9233849ac1d9a77bf65b56e1f6ce22d384
const collectPayments = String( translate( 'The ability to collect payments' ) ); | ||
const emailCustomerSupport = String( translate( 'Unlimited customer support via email' ) ); | ||
const liveChat = String( translate( 'Access to live chat support' ) ); | ||
const earnAndRevenue = String( translate( 'The ability to earn and revenue' ) ); |
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 assume that's supposed to be "earn ad revenue"?
const analyticsIntegration = String( translate( 'Google Analytics integration' ) ); | ||
const accessPlugins = String( translate( 'Access to more than 50,000 plugins' ) ); | ||
const seoTools = String( translate( 'Advanced SEO tools' ) ); | ||
const automatedBackups = String( translate( 'Automated site backupsand one-click restore' ) ); |
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.
Typo, "backupsand"
const accessPlugins = String( translate( 'Access to more than 50,000 plugins' ) ); | ||
const seoTools = String( translate( 'Advanced SEO tools' ) ); | ||
const automatedBackups = String( translate( 'Automated site backupsand one-click restore' ) ); | ||
const sftpAndDatabase = String( translate( 'SFTP and Database access' ) ); |
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 sure "Database" should be capitalized there.
automatedBackups, | ||
sftpAndDatabase, | ||
bestInClassHosting, | ||
liveChat, |
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.
Does liveChat
belong here? It's not listed for the other monthly plans, and seems to contradict https://wordpress.com/pricing/.
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.
Features were updated in the design vTj2aknM6tRUQndWtOcURT-fi-445%3A10488
action: 'removePlanAndAllSubscriptions', | ||
label: translate( 'Cancel my plan', { | ||
comment: | ||
'This button removes the active plan and all active Marketplace subscriptions on the site', |
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 am not sure what this has to do with marketplace subscriptions or removePlanAndAllSubscriptions
. I think it just removes the plan and nothing else.
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 was updated
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 still seeing the reference to "marketplace subscription" in the above comment, though.
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, I'm also seeing it. @ciprianimike maybe the commit isn't pushed?
Either way, I don't think we need this translator comment, since "Cancel my plan" has already been translated in other places since 2 years ago: https://translate.wordpress.com/projects/wpcom/-all-translated/282389/ . So let's remove this comment entirely :)
const shouldUseSiteThumbnail = | ||
isComingSoon === false && isPrivate === false && launchedStatus === true; | ||
const withinFirstYear = | ||
site.options.created_at !== undefined ? dateWithinLastYear( site.options.created_at ) : 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.
I am not sure what the site creation date has to do with anything.... Are you actually trying to figure out if their custom domain is more than a year old, maybe?
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 part is deprecated as the design changed and there are no more domains involved.
isComingSoon === false && isPrivate === false && launchedStatus === true; | ||
const withinFirstYear = | ||
site.options.created_at !== undefined ? dateWithinLastYear( site.options.created_at ) : false; | ||
const hasDomain = site.URL !== undefined && site.URL === site.options.unmapped_url ? false : true; |
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 seems to be checking whether the site has a custom primary domain? Doesn't seem like what you want here. My guess is you want to know something more along the lines of whether they used their domain credit and still have the domain they used it on.
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 part is deprecated as the design changed and there are no more domains involved.
@@ -211,6 +222,23 @@ class RemovePurchase extends Component { | |||
); | |||
} | |||
|
|||
shouldShowPlanWarning() { | |||
const { isAtomicSite, purchase } = this.props; | |||
return isPlan( purchase ) && ! isAtomicSite; |
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.
Why not for Atomic sites?
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 was fixed
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 did a quick run through the PR. I will have a look again but left some feedback.
const bestInClassHosting = String( translate( 'Best-in-class hosting' ) ); | ||
const addFreeSite = String( translate( 'An add-free site' ) ); | ||
const collectPayments = String( translate( 'The ability to collect payments' ) ); | ||
const emailCustomerSupport = String( translate( 'Unlimited customer support via email' ) ); | ||
const liveChat = String( translate( 'Access to live chat support' ) ); | ||
const earnAndRevenue = String( translate( 'The ability to earn and revenue' ) ); | ||
const premiumThemes = String( translate( 'Access to premium themes' ) ); | ||
const analyticsIntegration = String( translate( 'Google Analytics integration' ) ); | ||
const accessPlugins = String( translate( 'Access to more than 50,000 plugins' ) ); | ||
const seoTools = String( translate( 'Advanced SEO tools' ) ); | ||
const automatedBackups = String( translate( 'Automated site backupsand one-click restore' ) ); | ||
const sftpAndDatabase = String( translate( 'SFTP and Database access' ) ); | ||
const acceptPayments = String( translate( 'Accept payments in 60+ countries' ) ); | ||
const shippingCarriers = String( translate( 'Integration with top shipping carriers' ) ); | ||
const premiumDesign = String( | ||
translate( 'Premium design options customized for online stores' ) | ||
); | ||
const more = String( translate( 'and more' ) ); |
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.
Why do we need to cast this result to 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 the reason is that translate()
can be used to create interpolated React components, so its return type is TranslateResult
(this is similar to ReactNode
). This function returns string[]
, which is a sensible return value, since all of the translate()
calls above will in fact only return strings, but TypeScript doesn't know that. The cast forces the results to be strings in case they are not. We do this in a lot of places.
title={ String( translate( 'Checkout modal' ) ) } |
I learned recently that there's another way to do this which might be more semantic. The translate()
method accepts an options object as its second parameter and that object can have the textOnly
property. If set, TypeScript will know that the function will return a string.
alt={ translate( 'My Profile', { textOnly: true } ) } |
So this line could be:
const more = translate( 'and more', { textOnly: true } );
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.
But can't we return TranslateResult[] and avoid all the confusion? @sirbrillig
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.
Certainly, if you like. Sometimes that's the more useful fix.
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 leave that judgment call to you @ciprianimike :) I am ok either way too 👍
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.
@jdc91 I see this approach used all over the codebase so I would keep it as it is for consistency, this is the reason why I used it
@@ -0,0 +1,257 @@ | |||
import { |
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.
Have you thought about a different way to do this possibly by maintaining a matrix (2d array). So that these feature's are easier to maintain and are more readable?
//Plan Slugs
const BUSINESS_PLAN = "BUSINESS_PLAN";
const PREMIUM_PLAN = "PREMIUM_PLAN";
...
//Periods
const MONTHLY = "MONTHLY"
const YEARLY = "YEARLY"
const planMatrix = {
"BUSINESS_PLAN" : {
"MONTHLY" : [ bestInClassHosting, addFreeSite, collectPayments, emailCustomerSupport ],
"YEARLY" : [ ... ],
},
"PREMIUM_PLAN" : {
"MONTHLY" : [ ... ],
"YEARLY" : [ ... ],
},
...
}
And you can do a final processing on the resulting array to figure out the domain related stuff. :)
This can possibly reduce some repetitive logic as well.
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.
@ciprianimike: Can most of the typos and maintenance concerns be addressed by using and/or extending @automattic/calypso-products/plans-list and adding something along the lines of getCancellationFlowFeatures()
?
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 cancellation flow feature list is now integrate in the @automattic/calypso-products/plans-list as requested
$modal-breakpoint-tablet: 1070px; | ||
$modal-breakpoint-mobile: 480px; | ||
|
||
.dialog__content.remove-plan-dialog { |
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.
All of the code in this sass file can be moved under this class there is no need to repeat this class on 17 and 132 and 138 can also be brought within this class to maintain a clear inheritance structure.
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 request was addressed
align="left" | ||
/> | ||
<p>{ translate( 'If you cancel your plan, you will lose:' ) }</p> | ||
<FeaturesList /> |
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 : this can just be shown inline :)
*/ | ||
const buttons = [ | ||
{ | ||
action: 'cancel', |
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.
A bit of a nitpick, but does this action need to be "cancel"? Since the primary action here is to "cancel my plan", it can be misleading.
<ul className="remove-plan-dialog__list-plan-features"> | ||
{ planFeatures.map( ( feature, index ) => { | ||
return ( | ||
<li key={ index }> |
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 might be fine here, but in general, using index
as a key is an anti-pattern.
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.
Good find. In this case feature
should be a unique string so we can probably use that instead!
buttons={ buttons } | ||
className={ dialogClassName } | ||
isVisible={ isDialogVisible } | ||
onClose={ closeDialog } |
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 would be a good idea to include some Tracks events with any user actions in this dialog.
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.
Good catch! Yes please, let's add Tracks events so we can measure user interaction in this dialog 🙏
@@ -352,16 +380,29 @@ class RemovePurchase extends Component { | |||
|
|||
const wrapperClassName = classNames( 'remove-purchase__card', className ); | |||
const Wrapper = useVerticalNavItem ? VerticalNavItem : CompactCard; | |||
const getWarningDialog = () => { | |||
if ( this.shouldShowPlanWarning() ) { | |||
return this.renderPlanWarningDialog(); |
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.
Does the plan warning dialog cover the same information in the non-primary domain warning dialog below? Since you're overriding the dialog, I think we need to let users with the hasNonPrimaryDomainsFlag
flag and a custom domain know that their site will be redirected to the simple site address on removal.
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.
@JanaMW27 can you take a look at this case?
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.
Yes, here is the solution: pebzTe-gw-p2
Translation for this Pull Request has now been finished. |
…eeding an update (#67525) * Don't update up-to-date plugins * Don't update up-to-date plugins when retrying and on clicking on update Co-authored-by: Yashwin <[email protected]>
…67561) Co-authored-by: Daniel Rodriguez <[email protected]>
* allow the Add Subscribers screen to be revisited * fix typo * change styling to match mockup * simplify styles * simplify dynamic classnames * simplify dynamic classnames * use goToStep instead of actionUrl
- remove deprecated spec
* Correct uses of stylelint disable When using "stylelint-disable-line" with no scope, autofix does not work. As a result, we MUST scope every disable line in order for autofix to work on the rest of the file. * Replace standalone 'stylelint-disable' with 'stylelint-disable-line' * Fix uses of disable-next-line
…wp-calypso into update/pre-cancelation-modal Rebase trunk branch
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 |
I did a functional review of this, and it seems to work as expected now. I tested each of the plans on my test account. This included:
Some minor observations:
|
@JessBoctor there's a new PR since I corrupted this one (100 my bad). About your points:
|
Regarding point 3, the priority is to ship the modal, we can improve it once v1 is live |
I think something strange happened with the latest update of this branch. Github is showing 512 commits and 2,082 files changed: The latest commit has two parents (it's a merge, not a rebase), and its comments mention both "merge" and "rebase". If I check the lineage in The parent of the left side is 254d863 from 2022-09-19, the parent of the right side is 94dcdfa from 2022-08-29. I'm not yet sure if this represents an actual danger upon merging, or if it's only confusing github's change detection. My current hypothesis is that the branch was rebased, and then the rebased commit was merged with the unrebased branch (instead of being force pushed). Remedition would be to force push the most recent commit on the rebased side as the branch. fd19d9e |
@mreishus please refer to this PR #68067 as this was corrupted |
@ciprianimike: if this is no longer being used, and you don't want to try @mreishus's fix above, then let's close it. |
P2: pebzTe-6r-p2
#### Issue
1039-gh-Automattic/martech
Acceptance Criteria
1039-gh-Automattic/martech
Proposed Changes
Testing Instructions
Before
After
Pmtriv.mp4
Issue
Related to 1039-gh-Automattic/martech