Plans: Layout and text changes for Jetpack sites #2629
Conversation
richardmuscat
added
Jetpack
[Status] Needs Review
|
@scruffian -- is this something that you can help review or help me work out who can do so? |
| { hasDiscount ? this.translate( 'for first year' ) : plan.bill_period_label } ( | ||
| { plan.saving } | ||
| % { this.translate( 'savings' ) }) | ||
| </small> |
There was a problem hiding this comment.
there's some weird indentation inside this if, looks like it is using spaces instead of tabs.
There was a problem hiding this comment.
Fixed in df86daa
|
@richardmuscat great job for a first Calypso PR! |
| @@ -221,6 +221,12 @@ var PlansCompare = React.createClass( { | |||
| }, | |||
|
|
|||
| render: function() { | |||
| var compareString = 'Compare Plans'; | |||
There was a problem hiding this comment.
This should probably still be translated.
There was a problem hiding this comment.
Fixed: 84f7916
|
I am not sure if this approach is the tidiest way to achieve this. This introduces a lot of branching in different components based on whether or not the site is a jetpack site. It might be better to branch higher in the code, and then create specific components for Jetpack sites. Let me know if you want me to explain this idea in more detail, or if you think it's not going to work :) |
| </small> | ||
| </div> | ||
| ); | ||
| if ( site && site.jetpack ) { |
There was a problem hiding this comment.
What about creating a new component specifically for Jetpack plan prices? That way we don't need this big if/else.
richardmuscat
added
[Status] In Progress
[Feature] Plans & Upgrades
| @@ -221,6 +226,12 @@ var PlansCompare = React.createClass( { | |||
| }, | |||
|
|
|||
| render: function() { | |||
| var compareString = this.translate( 'Compare Plans' ); | |||
|
|
|||
| if ( this.props.selectedSite.jetpack ) { | |||
There was a problem hiding this comment.
We should check if this.props.selectedSite is defined: this.props.selectedSite && this.props.selectedSite.jetpack
There was a problem hiding this comment.
Done: 422d90b
|
@richardmuscat: code looks good to me, what's the best way to test this? It would be good to squash all those commits 😊 |
|
To test, visit calypso/plans/jetpack-site and you should see the plans Also test a wpcom site (ie not Jetpack) to make sure nothing changed there. |
|
Git allows you to squash multiple commits into one. It is seen as a good practice in Calypso to squash small commits like "fixing a typo" to the commit(if it is part of the PR) that introduced the typo. https://git-scm.com/book/en/v2/Git-Tools-Rewriting-History We can screenshare and I can show you how to!
👍 |
| @@ -1,32 +1,46 @@ | |||
| .plan-header .plan-price { | |||
| .plan-header .plan-price, | |||
There was a problem hiding this comment.
Duplicating classes like this will make the CSS really bloated. Can we use a mixin instead?
There was a problem hiding this comment.
We spoke about this and it seems like this approach is the simplest one for now. Trying to avoid premature optimization :)
richardmuscat
force-pushed
the
update/jetpack-plans-text
branch
2 times, most recently
from
0d2ac37 to
9c270eb
Compare
|
When testing I noticed that the placeholders on the plans page and the comparison page have three columns, but this layout only has two columns. Other than that LGTM 👍 |
scruffian
added
[Status] Ready to Merge
and removed
[Status] Needs Review
richardmuscat
force-pushed
the
update/jetpack-plans-text
branch
from
9c270eb to
cea05ea
Compare
Plans: Layout and text changes for Jetpack sites
We're trying a different approach with presenting plans for .org sites including: hiding the "Free" tier, showing the savings over buying individual plugins (VaultPress, Akismet, and PollDaddy), and updating the text descriptions of the plans themselves as well as the feature detail.
Screenshots of what the final result should look like:
Plan selection page:
Plan comparison page: