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

Plans: Color PlanThankYouCard based on plan #11865

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Mar 7, 2017

This PR is meant to colorize the plans thank you card, and use the PlanIcon component, based on the type of plan the user has purchased. It is an improvement that @rickybanister suggested.

You can view @rickybanister's mocks here: p6TEKc-105-p2

Here are some screenshots for how things currently look in this PR.

Note: In all screenshots, you'll notice the name of the plan and pricing is off. This is because I manually changed the plan type to get the screenshots. Further, the auto-config list below the thank you card should only show for Jetpack sites in the development environment. Lastly, you'll notice that there the background for the icons doesn't stand out. We'll need to fix this before merging.

Free Plan

screen shot 2017-03-07 at 1 23 16 pm

Personal Plan

screen shot 2017-03-07 at 1 05 02 pm

Premium Plan

screen shot 2017-03-07 at 1 24 38 pm

Business Plan

screen shot 2017-03-07 at 1 27 46 pm

To test:

  • You can test the WPCOM plans flow by creating a new user and following the NUX flow
  • For Jetpack, you can purchase a plan for your Jetpack site, and then you should land on the auto-config flow
  • You can test previous purchases by going to: http://calypso.localhost:3000/checkout/thank-you/:siteId:/:receiptId:

@ebinnion ebinnion added .org Plans [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Status] In Progress [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR labels Mar 7, 2017
@ebinnion ebinnion self-assigned this Mar 7, 2017
@matticbot
Copy link
Contributor

@matticbot matticbot added the [Size] L Large sized issue label Mar 7, 2017
@beaulebens
Copy link
Member

Neat! A couple things I noticed from the screenshots above vs the mocks:

  1. Shouldn't they all have a circle around the pencil/pen icon?
  2. On the Personal Plan one, there's an icon/emoji floating over the pen icon, but I wouldn't expect that to be the case (I'd expect the pen/circle combo to be the "highest" thing in the design).
  3. I think the checkmarks down the bottom are supposed to match the main color of the specific plan (yellow, purple, etc)

I'll let @rickybanister handle the rest :)

@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 7, 2017

Shouldn't they all have a circle around the pencil/pen icon?

I had noticed that and mentioned it in the top comment. @rickybanister suggested that either that header or the icon would need to be darkened by 10%, and that he'd need to try it out to know.

On the Personal Plan one, there's an icon/emoji floating over the pen icon, but I wouldn't expect that to be the case (I'd expect the pen/circle combo to be the "highest" thing in the design).

Good catch. I'll look into that!

I think the checkmarks down the bottom are supposed to match the main color of the specific plan (yellow, purple, etc)

👍 . I'll handle this in another PR since it's another component that handles the auto-config.

@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Mar 7, 2017
@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 7, 2017

I pushed a commit so that the background icons are in the background and do not overlap the plan icon or text.

@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 7, 2017

Updated screenshots after making sure the PlanIcon has a background and that the background icons stay in the background.

screen shot 2017-03-07 at 4 43 11 pm
screen shot 2017-03-07 at 4 43 37 pm
screen shot 2017-03-07 at 4 40 53 pm
screen shot 2017-03-07 at 4 41 52 pm

@ebinnion ebinnion force-pushed the update/plans-thank-you-icons-colors branch from bd0fbef to 93f814c Compare March 7, 2017 22:54
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 7, 2017
@rickybanister
Copy link

Coming along nicely—I haven't tested yet, but do we have the progress bar implemented while we set things up?

@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 7, 2017

I have not yet implemented the progress bar. I plan on doing that in another PR.

@rickybanister
Copy link

rickybanister commented Mar 8, 2017

This seems ready to merge (design-wise) in that case.

@rickybanister rickybanister removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Mar 8, 2017
@ebinnion ebinnion force-pushed the update/plans-thank-you-icons-colors branch from 93f814c to b0e4df4 Compare March 8, 2017 18:45
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 8, 2017
@ebinnion ebinnion force-pushed the update/plans-thank-you-icons-colors branch from b0e4df4 to 6021111 Compare March 8, 2017 18:46
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label Mar 8, 2017
@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 8, 2017

At this point, I've finished rebasing after #11702 went in. I also made sure to do some testing and grab screenshots for how this will change the current implementation of the WPCOM plan thank you card usage.

Here are those:

screen shot 2017-03-08 at 12 55 55 pm
screen shot 2017-03-08 at 12 56 17 pm
screen shot 2017-03-08 at 12 54 54 pm

cc @danhauk @drewblaisdell @gziolo who seem to have worked on PlanThankYouCard and ThankYouCard so far.

cc @lezama and @enejb for code review.

@ebinnion ebinnion 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 8, 2017
@ebinnion ebinnion requested review from lezama and enejb March 8, 2017 19:00
@rickybanister
Copy link

Beautiful!

cc @shaunandrews since this was his original design.

@lezama
Copy link
Contributor

lezama commented Mar 8, 2017

Code looks good to me.

Probably not from this PR but I noticed that it works with a site_id and a receiptId of another site that I own. Also confirmed that it doesn't work with a random receiptId.

Copy link
Contributor

@lezama lezama left a comment

Choose a reason for hiding this comment

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

🚢

@gziolo
Copy link
Member

gziolo commented Mar 9, 2017

Code changes look good 👍

Probably not from this PR but I noticed that it works with a site_id and a receiptId of another site that I own.

Yes, it needs to behave this way to make sure it also works with the PayPal flow where we redirect from the different site to the thank you page :)

@danhauk Should we use different color also for domain purchases? We can create a follow up if we agree on something.

}

.plan-thank-you-card.is-free-plan {
.thank-you-card{
Copy link
Member

@gziolo gziolo Mar 9, 2017

Choose a reason for hiding this comment

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

I'm wondering if we can create mixin that would take care of all color changes to simplify this file.

Something along those lines:

.plan-thank-you-card.is-free-plan {
    @include thank-you-card-color( $blue-medium );
}

@gziolo
Copy link
Member

gziolo commented Mar 9, 2017

Probably not from this PR but I noticed that it works with a site_id and a receiptId of another site that I own.

@lezama Now I see what you mean, you can enter whatever you want as site_id and it is still going to work.

@matticbot matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Mar 9, 2017
@gziolo
Copy link
Member

gziolo commented Mar 9, 2017

I introduced mixin with a314f20 to save some lines in CSS file :) If you don't like it, you can simply remove my commit :)

I tested all 3 paid plans and it all works as expected.

I have only one concern. We probably should display different copy when upgrading plan. I don't think this one fits well when you upgrade from Premium to Business:

screen shot 2017-03-09 at 09 28 18

In such case the site most likely isn't new. Should we open a follow up PR? /cc @ranh

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@ranh
Copy link
Contributor

ranh commented Mar 9, 2017

Thanks for the ping @gziolo. I agree that this message doesn't make much sense if you're getting a plan for an existing site (either going from free to paid or upgrading to higher level plan).

The focus of this part of the message is not the plan at all, rather the "new site". So it seems this is specific to NUX.

In fact the plan is described as just something that had to be "taken care of", which is a very strange thing to tell users here. Should we try for a more positive message?

I can suggest different copy if needed, but just want to get a better idea of what we would want to say.

@rickybanister
Copy link

@gziolo and @danhauk and @ranh I'd suggest the 'free' blue-medium color for the domain purchase, and perhaps we put back in place the checkmark icon (or perhaps a domain-specific icon) for those. Our focus was to make the card match the plan and didn't consider the domain.

@ebinnion
Copy link
Contributor Author

ebinnion commented Mar 9, 2017

@gziolo Very nice with the mixin! Great idea. ;)

I'm going to go ahead and merge as the copy changes are pre-existing and this card should only be showing for new WPCOM users that purchase a WPCOM plan. We can address domain-specific purchase styling and copy in another PR.

@ebinnion ebinnion merged commit 96f50bd into master Mar 9, 2017
@ebinnion ebinnion deleted the update/plans-thank-you-icons-colors branch March 9, 2017 15:05
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Mar 9, 2017
@gziolo
Copy link
Member

gziolo commented Mar 9, 2017

We can address domain-specific purchase styling and copy in another PR.

Yes, we will discuss if stay with green or change to blue for /domains flow.

if ( isNewUser && wasDotcomPlanPurchased ) {

I figured out why I was seeing new thank you page when upgrading plan. We check account creation date. I don't think it is going to happen to users, it would be rather super edge case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Components [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. [Size] L Large sized issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants