-
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
Purchase Management: add new cancelation modal #68067
base: trunk
Are you sure you want to change the base?
Conversation
Here is how your PR affects size of JS and CSS bundles shipped to the user's browser: App Entrypoints (~815 bytes added 📈 [gzipped])
Common code that is always downloaded and parsed every time the app is loaded, no matter which route is used. Sections (~4210 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 (~813 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. |
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.
@dzver I pushed a fix for this. |
I did a quick test and saw that most of the issues from my comment at #67090 (review) are still present (in particular the first, "We seem to have a double confirmation", and the third, "This is being applied to Jetpack too")? Also noticed that the messaging seems backwards between refundable and non-refundable cancellations:
Seems like it should be the other way around. Also in the above screenshots, I thought "Best in-class hosting" wasn't supposed to be there anymore (since they aren't losing their hosting)? And in the first screenshot, "custom domain as primary"... I saw that message even though I don't have a custom domain. Also, even if I did have one, the wording seems pretty confusing to me. |
* Plan cancellation dialog. | ||
*/ | ||
return ( | ||
<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.
This is still missing Tracks events: #67090 (comment)
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.
@michaeldcain could you please verify the tracking events added here https://github.com/Automattic/wp-calypso/pull/68067/files/80fea556de0d33e45084d1132c5d68428240734b#diff-22f6c96a70e669c1ee86915b052fe607759a7d300568afe8a92d9646c37860d0R33 ?
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.
@michaeldcain could you please reviews the changes above?
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: sorry I missed this ping. Those events look good to me.
👋 From my initial review It looks like the issues I raised have all been resolved.
For posterity, I was testing on my 14" MBP A2442. MacOS version 12.5 and Chrome version 105.0.5195.125. Display resolution is set to |
We need the wording: If you cancel your plan, once it expires, you will lose: |
I just want to confirm, is this step supposed to be visible as a second step after clicking the Cancel subscription and refund? (the PR needs rebase, these buttons are now with a different color and copy) CC @JanaMW27 |
@dzver that step will be skipped, I discussed it yesterday with @JanaMW27 and worked on the fix |
After some scope and edge cases considerations, we have decided to keep this screen ONLY for users within the refund window. |
7cc48b6
to
300c3a2
Compare
@DavidRothstein I updated the PR, the extra step will be present as per the comment: #68067 (comment)", I updated the checks for the site having primary domain and removed the hosting. I tested Jetpack subscriptions via jurassic-ninja and it should be fine. |
I did another round of brief testing.
For me, the modal is still appearing for Jetpack. Note that you do need to purchase a Jetpack plan to see this (or I think they're calling it "bundle" now) -- for example Jetpack Security, as shown in my original screenshot. A Jetpack product does not have the problem.
I'm not seeing this text appear at all anymore (even when cancelling a nonrefundable subscription, where it's supposed to appear)?
The behavior I see now doesn't seem to match that comment -- the extra step is present for me in both cases (not just the refundable case)? Anyway, I guess I can see the logic of having a two-step confirmation for now (maybe even in both cases). The two steps do provide different information. However, there is also a fair amount of duplicate information if you just keep the second screen exactly as is (for example, the "Plan Features" link on the second screen completely duplicates the first screen). And the biggest thing that feels extremely disjointed is jumping in and out of the modal. You click to cancel, see a modal, click to continue, jump out of the modal, click to continue, and now go into a different kind of modal (full-screen-style) where you get a survey, etc... Couldn't we have both steps of the confirmation inside the modal instead?
It still seems confusing to me. The full context here being:
I don't think most people will understand what that means. Whereas the current wording that this is replacing (#39909 shows an almost-accurate screenshot) uses more basic terminology. Couldn't we use something more like that? Perhaps:
It's longer, but this is a complicated and important concept, and it should be OK to explain it using enough words to make it clear. |
e00eef7
to
63a482e
Compare
I pushed a fix, before only Jetpack products were excluded, now alsp Jetpack plans. |
This text should be now visible only for all modals but subscriptions in the refund window. |
Set subtitle as 'If you cancel your plan, once it expires, you will lose:' in all the modals but the one in the refund window
b1beb7e
to
d635122
Compare
I tested again and am still seeing some of the previously-mentioned problems, in particular:
It's currently happening for any plan cancellation within the refund window (the link you provided is for the case where there's a plan and domain, which is pretty common in its own right, but is not the only case). Here's a video showing the behavior for cancelling just a plan (no domain involved): cancel-refundable-plan.mp4What is the rationale for fixing that flow as a followup, rather than launching this with the intended experience in place to begin with? If the goal is to get something out sooner, would it be better to just release the modal for the non-refundable case only to start (and then hold off on the refundable case until it's actually ready)? Regarding the non-refundable case, this pull request is indeed replacing the old screen with the new modal, which is good. Comparing them below, I do see two things that have gone missing that may be important:
Old cancel confirmation: New cancel confirmation: |
This is being broken down into smaller diffs per this post: pebzTe-oH-p2 I'm going to make this a draft PR so that we'll get pinged if it ever gets changed back. |
Translation for this Pull Request has now been finished. |
Pemise
The PR #67090 is corrupted, I opened this new and addressed the change requests.
P2: pebzTe-6r-p2
#### Issue
1039-gh-Automattic/martech
Acceptance Criteria
1039-gh-Automattic/martech
Proposed Changes
Testing Instructions
Edge cases:
Before
After
Pmtriv.mp4
Issue
Related to 1039-gh-Automattic/martech