Skip to content
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

Purchases: Update the cancellation confirmation screen for domains #4495

Merged
merged 8 commits into from
Apr 5, 2016

Conversation

Tug
Copy link
Contributor

@Tug Tug commented Apr 3, 2016

Reimplement the "confirm cancel puchase" form for domains on the client.
Previously the form was returned by the API and injected into Calypso.

Testing Instructions

Reviews

  • Product Review
  • Code Review

@Tug Tug self-assigned this Apr 3, 2016
@Tug Tug added [Status] In Progress [Feature] Purchase Management Related to managing purchases such as subscriptions, plans, history, auto-renew, cancellation, etc. labels Apr 3, 2016
@Tug Tug added this to the Amber: Sprint milestone Apr 3, 2016
@@ -0,0 +1,25 @@
.confirm-cancel-domain {
// the following is a hack so the dropdown selector is not cropped when it overflows its container
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't found a nice way to fix this issue yet. If we remove this line the dropdown is cropped as shown here
image
A simple solution would be to use a select element instead of the SelectDropdown component.

@Tug Tug added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Apr 4, 2016
paths.confirmCancelPurchase(),
'Confirm Cancel Purchase'
paths.confirmCancelDomain(),
'Confirm Cancel Domain'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we use titles.confirmCancelDomain here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could yeah, is it clearer what information is sent to the analytic engine though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want to pass a translated string to recordPageView.

@scruffian
Copy link
Member

I see this when I try to cancel a business plan:

Error: ConfirmCancelDomain.render(): A valid ReactComponent must be returned. You may have returned undefined, an array or some other invalid object.

@scruffian scruffian force-pushed the update/confirm-cancel branch from 95c2397 to 69cb2de Compare April 4, 2016 15:44
return false;
}

if ( [ 'other_host', 'transfer' ].indexOf( selectedReason.value ) >= 0 ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super minor, but you could replace this clause and the tail return with

return [ 'other_host', 'transfer' ].indexOf( selectedReason.value ) === -1;

@drewblaisdell drewblaisdell added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 4, 2016
@Tug Tug force-pushed the update/confirm-cancel branch from c68f6fb to 62e8796 Compare April 4, 2016 21:31
function loadEndpointForm( selectedPurchase, onSuccess ) {
const { id, productId } = selectedPurchase;

wpcom.getCancellationPageHTML( id, productId, ( error, response ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove this endpoint from wpcom-undocumented/lib/unpublished.js.

@Tug
Copy link
Contributor Author

Tug commented Apr 4, 2016

I believe I took care of most of your concerns.
Rebased after #4475 was merged so the business issue should have been taken care of

@Tug Tug 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 Author Reply labels Apr 4, 2016
@drewblaisdell drewblaisdell force-pushed the update/confirm-cancel branch 2 times, most recently from 9876823 to e49047b Compare April 4, 2016 23:34
@drewblaisdell
Copy link
Contributor

This works well, and the code looks good to me. I updated the commit history with a couple minor changes: CSS naming fixes, as well as removing Undocumented.prototype.getCancellationPageHTML. Make sure to fetch/reset locally so as not to wipe out these changes. :)

@scruffian
Copy link
Member

This looks good. I noticed #4518 while testing, but we should fix that in a different PR.

LGTM 👍

@scruffian scruffian added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Apr 5, 2016
Tug added 8 commits April 5, 2016 09:10
Reimplement confirm cancel puchase form for domain on the client.
Previously the form was returned by the API and injected into Calypso
Edited the logic of `isCancellable( purchase )`.
A purchase can have its `isCancellable` property set to `false`
when a request to cancel have already been made for instance.
@scruffian scruffian force-pushed the update/confirm-cancel branch from e49047b to 373ac2b Compare April 5, 2016 08:10
@scruffian scruffian merged commit c6d004e into master Apr 5, 2016
@Tug Tug deleted the update/confirm-cancel branch April 5, 2016 09:07
scruffian added a commit that referenced this pull request Apr 5, 2016
Purchases: Update the cancellation confirmation screen for domains
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Purchase Management Related to managing purchases such as subscriptions, plans, history, auto-renew, cancellation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants