-
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
Plans: Add plan features to Plan Overview page #1565
Conversation
@@ -86,3 +88,17 @@ | |||
.plan-status__time-until-expiry { | |||
font-weight: 700; | |||
} | |||
|
|||
.plan-features__feature-description { |
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.
Should this CSS live in the same directory as PlanFeatures
?
Only some of these features are available in WordPress.com Premium, so we need to show/hide features based on the plan the user is in. |
I don't think we can offer an upsell from a Premium Trial to a Business Trial; the user has to buy Business in this case. |
Done in cf7c244. |
@@ -0,0 +1,27 @@ | |||
@mixin plan-overview-button { |
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.
@scruffian it looks like we should use @extend for mixins that don't accept an argument (docs).
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 giving his some more thought, I think sharing this style using classes is preferable to either a mixin or extend:
.plan-overview {
.plan-feature__button,
.plan-status__button {
flex-shrink: 0;
margin-left: 15px;
min-width: 150px;
text-align: center;
@include breakpoint( "<480px" ) {
margin: 15px 0 0 0;
width: 100%;
}
}
}
Rationale:
- This is easier to grok than a mixin/extend. Defining the styles for
%plan-overview-button
in thePlanOverview
stylesheet and including them with@extend
elsewhere makes it difficult to know where the styles are used. - This probably results in a smaller CSS bundle.
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.
This approach makes sense too, but I didn't like the idea of having rules for one component live inside another. I wonder what @mtias thinks...
3b8bb16
to
189e06b
Compare
@include plan-overview-row(); | ||
} | ||
|
||
.plan-features__feature-button { |
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.
We should update the names here to match the component name, e.g. .plan-feature__button
78b0d4e
to
181327e
Compare
These features don't have calls to action.
This makes sure we're using them consistently.
…cessibility reasons
…add a left margin to create space between the button and the text
…mponent files targetting their respective components, and keep the code DRY
…header more readable
… make it obvious that this is a simple component
… Features component
c989483
to
8ac0528
Compare
I've made a few small adjustments and rebased against master. This is ready to go. |
Plans: Add plan features to Plan Overview page
{ button && | ||
<Button | ||
className="plan-feature__button" | ||
href={ button.href } |
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.
Talking a bit with @breezyskies... can we try a compact
button here? I think it'd work a bit better in terms of weight.
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.
Just to loop back, PR started in #1820 for this.
This pull request addresses part of #1151 by adding a list of plan features at the bottom of the
Plan Overview
page. It also displays a nudge to upgrade to Business when the current plan on trial is Premium:Testing instructions
git checkout add/plan-overview-features
and start your serverStart Free Trial
button on thePlans
pageStart 14 Day Free Trial
button on theSecure Payment
pagePlans
page which should now display thePlan Overview
variantReviews