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: Add expired state to PlanStatus #1780

Merged
merged 7 commits into from
Dec 22, 2015

Conversation

drewblaisdell
Copy link
Contributor

Addresses part of #1471.

screen shot 2015-12-17 at 5 23 38 pm

Testing

  • Visit http://calypso.localhost:3000/plans/:site for a site with a trial that is in the grace period (within three days of expiring).
  • If there are three days left, assert that the notice reads 'EXPIRED TODAY', and that the remaining time text reads 'Plan features will be removed in 3 days'.
  • If there are two days left, assert that the notice reads 'EXPIRED 1 DAY AGO', and that the remaining time text reads 'Plan features will be removed in 2 days'.
  • If there is one day left, assert that the notice reads 'EXPIRED 2 DAYS AGO', and that the remaining time text reads 'Plan features will be removed in 1 day'.
  • If it is the day of the expiry, assert that the notice reads 'EXPIRED 3 DAYS AGO', and that the remaining time text reads 'Plan features will be removed momentarily'.

Reviews

  • Code
  • Product

@drewblaisdell drewblaisdell self-assigned this Dec 18, 2015
@drewblaisdell drewblaisdell added this to the No-cc Free Trials: v1 milestone Dec 18, 2015
@drewblaisdell drewblaisdell changed the title Add/plan status expired state Plans: Add expired state to PlanStatus Dec 18, 2015
@@ -7,7 +7,7 @@ function createSiteSpecificPlanObject( plan ) {
return {
currentPlan: Boolean( plan.current_plan ),
expiry: plan.expiry,
expiryMoment: moment( plan.expiry ),
expiryMoment: moment( plan.expiry ).startOf( 'day' ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Tug The issue we encountered during the meetup is simplified a little bit by:

  • Ensuring that all moments (those in this assembler and the current moment calculated with moment()) are set to the start of the day.
  • Using moment().startOf( 'day' ) instead of moment().set( { hour: 0, ... } ) each time.

@drewblaisdell drewblaisdell force-pushed the add/plan-status-expired-state branch from c2e90b8 to f195578 Compare December 18, 2015 22:56
@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 Dec 18, 2015
@drewblaisdell
Copy link
Contributor Author

This has been rebased to pull in changes from #1708 and is ready for review.

return true;
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about the following simpler approach?

export function isInGracePeriod( plan ) {
 return ( getDaysUntilUserFacingExpiry( plan ) <= 0 ) && ( getDaysUntilExpiry( plan ) >= 0 );
}

@stephanethomas
Copy link
Contributor

I guess this shouldn't happen since the subscription would probably have been removed at that point (well, I'm not quite sure about that) but I've noticed the following display bug once the grace period is over:

screenshot

@stephanethomas
Copy link
Contributor

I added a few remarks but otherwise this works as advertised.

@stephanethomas stephanethomas 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 Dec 19, 2015
@drewblaisdell drewblaisdell force-pushed the add/plan-status-expired-state branch 4 times, most recently from 5cf6219 to 4625845 Compare December 21, 2015 18:02
@drewblaisdell
Copy link
Contributor Author

Thanks for looking at this, @stephanethomas. I updated the commits to address your feedback with the following changes:

  • Updated the syntax of isInGracePeriod.
  • Switched to use days instead of daysUntilUserFacingExpiry in renderNotice.
  • Updated the syntax of the notice text definition in renderNotice.
  • Updated PlanStatusProgress to handle the case where the trial was not removed. Ideally, users shouldn't ever be in this state, but it is easy enough to handle.

@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] Needs Author Reply labels Dec 21, 2015
@ghost
Copy link

ghost commented Dec 22, 2015

LGTM - only concern is the styling.
Even with a max-viewport window, the text takes up two lines:
screen shot 2015-12-21 at 4 58 33 pm
It looks a lot better in the mockup above, with the text on a single line (WordPress.com Premium Free Trial)

@Tug Tug force-pushed the add/plan-status-expired-state branch 2 times, most recently from 7a59eb6 to b6780fb Compare December 22, 2015 15:37
@Tug
Copy link
Contributor

Tug commented Dec 22, 2015

Some margin must have been added or something, so it wasn't fitting into one line in english.
I fixed that by decreasing the font size a little bit

Also added some comment and reviewed the code 👍 LGTM

@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 Dec 22, 2015
@stephanethomas
Copy link
Contributor

I really don't think we should change the size of the font to avoid having two lines of text. I bet it's longer in French and in other languages. If this is really an issue, we should prevent it from wrapping using the appropriate CSS property and take advantage of the flexbox layout to move the button below if there is no space left.

@mikeshelton1503
Copy link
Contributor

For the font-size, the original intended size was 21px so that should fix the English version at max-width. Sorry I should have indicated that originally.

For other languages it may still wrap but I'm ok with this. Stephane's suggestion could work too but we're not gaining too much with that method because the overall height of the section would be taller so it would just be a trade off of evils.

@drewblaisdell drewblaisdell force-pushed the add/plan-status-expired-state branch from b6780fb to dac331f Compare December 22, 2015 17:25
@drewblaisdell drewblaisdell force-pushed the add/plan-status-expired-state branch from dac331f to f4eb05f Compare December 22, 2015 17:26
drewblaisdell added a commit that referenced this pull request Dec 22, 2015
@drewblaisdell drewblaisdell merged commit a09ab70 into master Dec 22, 2015
@drewblaisdell drewblaisdell deleted the add/plan-status-expired-state branch December 22, 2015 17:35
@drewblaisdell
Copy link
Contributor Author

I updated @Tug's commit to change the font size to 21px. 👍 The font size isn't an issue related to this PR, so we can handle any other changes separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants