-
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
Payments: scaffold the checkout pending page, the 2nd attempt. #23827
Conversation
page( '/' ); | ||
|
||
showErrorNotice( | ||
translate( "Sorry, we've encountered an unknown problem. Please try again later." ) |
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.
Hi! I've found a possible matching string that has already been translated 19 times:
translate( 'We've encountered a problem. Please try again later.' )
ES Score: 12
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
page( '/' ); | ||
|
||
showErrorNotice( | ||
translate( "Sorry, we've encountered an unknown problem. Please try again later." ) |
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.
Hi! I've found a possible matching string that has already been translated 19 times:
translate( 'We've encountered a problem. Please try again later.' )
ES Score: 12
Help me improve these suggestions: react with 👎 if the suggestion doesn't make any sense, or with 👍 if it's a particularly good one (even if not implemented).
Hi @southp -- going through the various screens, here are a couple of quick notes re: the copy:
|
9d53ff6
to
999a8be
Compare
@benhuberman Thanks for the feedback! I've updated the two screens as: Does it look good to you? |
1365c79
to
99ea650
Compare
999a8be
to
ef4821d
Compare
// See the explanation in https://github.com/Automattic/wp-calypso/pull/23670#issuecomment-377186515 | ||
if ( ORDER_TRANSACTION_STATUS.FAILURE === processingStatus ) { | ||
// Bring the user back to the homepage in this case. | ||
page( '/' ); |
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 having second doubts about this - maybe the plans page is better?
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 playing a bit more, I agree with you. Since the whole flow is initiated from the plan page, redirecting back there is much less confusing. I've updated it in 9fc71cf. The case of unknown status has been updated as well, which I think the same reasoning applied :)
to the controller.
notice would be eliminated before showing.
payment cancellation or technical errors.
9fc71cf
to
53b7e6a
Compare
Looks good to me, @southp! |
Summary
This PR renovates #23670 so that the pending checkout page routes according to the new state tree defined in #23822.
Test Plan
dispatch( { type: 'ORDER_TRANSACTION_SET', orderId: 12345, transaction: { processingStatus: 'ORDER_TRANSACTION_STATUS_SUCCESS' } } );
and you should see:dispatch( { type: 'ORDER_TRANSACTION_SET', orderId: 12345, transaction: { processingStatus: 'ORDER_TRANSACTION_STATUS_ERROR' } } );
. You should be redirected to the checkout page like this:dispatch( { type: 'ORDER_TRANSACTION_FETCH_ERROR', orderId: 12345, error: {} } );
and you should see the same page like above.dispatch( { type: 'ORDER_TRANSACTION_SET', orderId: 12345, transaction: { processingStatus: 'ORDER_TRANSACTION_STATUS_FAILURE' } } );
and you should be redirected back to homepage.dispatch( { type: 'ORDER_TRANSACTION_SET', orderId: 12345, transaction: { processingStatus: 'ORDER_TRANSACTION_STATUS_UKNOWN' } } );
and you should be redirected back to homepage with an error notice like this: