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.

Sign up for GitHub

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

Jump to bottom

Checkout: Thank You: Update DomainMappingDetails with new copy #3715

Merged
merged 5 commits into from
Mar 7, 2016
Merged

Checkout: Thank You: Update DomainMappingDetails with new copy #3715

merged 5 commits into from
Mar 7, 2016

Conversation

drewblaisdell
Copy link
Contributor

Requires patch D1216-code

screen shot 2016-03-01 at 5 38 23 pm

Testing

  • Visit /domains/add/mapping/:site.
  • Add a domain mapping item for an existing domain.
  • Proceed through checkout.
  • Assert that you are landed on a thank you page that resembles the screenshot above.

Testing instructions for a mapped domain for which we can link to the registrar's site are available with the patch.

  • Code review
  • Product review

@drewblaisdell drewblaisdell self-assigned this Mar 2, 2016
@drewblaisdell drewblaisdell force-pushed the update/thank-you-domain-mapping branch 2 times, most recently from 69aca66 to 673b90b Compare March 2, 2016 01:31
stephanethomas
stephanethomas reviewed Mar 2, 2016
View reviewed changes
client/components/purchase-detail/README.md
@@ -33,3 +33,4 @@ export default React.createClass( {
- *isPlaceholder* (boolean) – determines whether or not to render shimmering placeholders
- *target* (string) – target passed as the `target` prop for the `Button`
- *title* (string) – string used as the text of the heading
- *requiredText* (boolean) – adds a notice to the top, determines the text in that notice
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be wrong but I think we display the same notice everywhere. If that's really a case we could just update this component to accept a boolean that will trigger the display of this notice. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

According to #3713 we need to display different notices so forget about my comment :).

@stephanethomas stephanethomas mentioned this pull request Mar 2, 2016
Merged
2 tasks
@drewblaisdell drewblaisdell force-pushed the update/thank-you-domain-mapping branch 3 times, most recently from 1fba680 to 3f07ce2 Compare March 3, 2016 21:58
@drewblaisdell drewblaisdell 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 Mar 3, 2016
@fabianapsimoes
Copy link
Contributor

I'm not quite sure about the alignment of the copy in mobile. What if we had it all aligned to the left? cc @breezyskies @mikeshelton1503 for input

screen shot 2016-03-04 at 12 18 08

Apart from that, product 👍

Tug
Tug reviewed Mar 4, 2016
View reviewed changes
client/components/purchase-detail/index.jsx
@@ -70,6 +70,7 @@ PurchaseDetail.propTypes = {
description: React.PropTypes.oneOfType( [
React.PropTypes.array,
React.PropTypes.string,
React.PropTypes.object
Copy link
Contributor

Choose a reason for hiding this comment

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

So I guess it can be anything? Would be cool to add a custom propTypes for objects returned by this.translate (not for this PR I mean)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! However, I think only the latter two are returned by this.translate - the addition here is for when description is a single ReactElement.

@Tug
Copy link
Contributor

Tug commented Mar 4, 2016

Code 👍

@Tug
Copy link
Contributor

Tug commented Mar 4, 2016

I'm moving it to Ready to Merge.
@breezyskies you can change the label if you disagree ;)

@Tug Tug 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 Mar 4, 2016
@mikeshelton1503 mikeshelton1503 added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Ready to Merge labels Mar 4, 2016
@mikeshelton1503
Copy link
Contributor

I'm not quite sure about the alignment of the copy in mobile. What if we had it all aligned to the left?

Agreed, it's too much copy for centered. Let's keep it left-aligned at all screen sizes.

@fabianapsimoes fabianapsimoes 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 Mar 4, 2016
@drewblaisdell drewblaisdell force-pushed the update/thank-you-domain-mapping branch from 3f07ce2 to 1d82714 Compare March 4, 2016 18:58
@drewblaisdell
Copy link
Contributor Author

👍 I added a commit to align the text in DomainMappingDetails to the left. Here's a screenshot.

@drewblaisdell drewblaisdell added [Status] Ready to Merge [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 [Status] Ready to Merge labels Mar 4, 2016
@drewblaisdell drewblaisdell force-pushed the update/thank-you-domain-mapping branch from 1d82714 to 8a3e52c Compare March 4, 2016 20:10
@scruffian scruffian force-pushed the update/thank-you-domain-mapping branch from 8a3e52c to 410ca8e Compare March 7, 2016 13:05
@scruffian
Copy link
Member

I updated 8a3e52c so that the button isn't left aligned:

screen shot 2016-03-07 at 13 06 00

If you're good with this change then this 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 Mar 7, 2016
Tug added a commit that referenced this pull request Mar 7, 2016
@Tug
Merge pull request #3715 from Automattic/update/thank-you-domain-mapping
edb0bcb 
Checkout: Thank You: Update `DomainMappingDetails` with new copy
@Tug Tug merged commit edb0bcb into master Mar 7, 2016
@Tug Tug deleted the update/thank-you-domain-mapping branch March 7, 2016 15:11
This was referenced Mar 29, 2020
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@drewblaisdell drewblaisdell

Labels
Post Checkout [Type] Task
Projects
None yet
Milestone
Amber: Sprint
Development

Successfully merging this pull request may close these issues.
7 participants
@drewblaisdell @fabianapsimoes @Tug @mikeshelton1503 @scruffian @stephanethomas @nylen