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

Me: Adds ability to toggle holiday snow #1138

Merged
merged 2 commits into from
Dec 1, 2015
Merged

Conversation

ebinnion
Copy link
Contributor

@ebinnion ebinnion commented Dec 1, 2015

In #1123, @jeherve reported that the holiday snow option was not displayed within /me. This PR allows a user to toggle holiday snow.

Since holiday snow only displays from 12-1 to 1-4, we could also feature flag or time flag this option.

Instead of presenting a checkbox that allowed a user to "disable", as opposed to "enable" a feature, I modified the /me/settings endpoint in r127611-wpcom to reverse the logic for holiday snow.

To test:

  • Checkout update/me-holiday-snow
  • On a WordPress.com site, go to $site/wp-admin/options-general.php and ensure holiday snow is turned on
  • Go to /me/account
  • Toggle the checkbox for holiday snow a few times and ensure that the holiday snow displays accordingly

@ebinnion ebinnion added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account. labels Dec 1, 2015
@ebinnion ebinnion self-assigned this Dec 1, 2015
@Viper007Bond
Copy link
Contributor

Since holiday snow only displays from 12-1 to 1-4, we could also feature flag or time flag this option.

+1 to make for a cleaner UI the rest of the year. Let's not let our options interface get out of control.

@lancewillett
Copy link
Contributor

Testing notes:

  1. User who hadn't touched the option before seems to work exactly as expected. Logging into Calypso /me/account the box is checked and snow works as expected on blogs like http://en.blog.wordpress.com/
  2. User who previously had the option turned off before — holidaysun user meta value of "0" (string) — the UI in /me/account should reflect that and have it turned off. In testing, I noticed a user I had previously turned snow off (in old WP.com interface) now has it on. Those users could just all go turn it off, though — but it'd be nice to respect previous settings if possible.

@lancewillett
Copy link
Contributor

I can confirm that the user setting overrides a site-specific setting, as expected.

@ebinnion
Copy link
Contributor Author

ebinnion commented Dec 1, 2015

I just pushed up another commit that puts the holiday snow checkbox behind a date check. It should also be future-proof. 😄 I tested by setting the year for the startDate and endDate variables to year 2014.

@lancewillett The API still uses the same meta value as holidaysun did, holidaysnow is just the negation of that. Ex. holidaysnow === ! holidaysun.

Also, for what it's worth, when holidaysun was truthy, I believe that that actually turns off the snow effect. So, I would expect a value of 0 for holidaysun to allow snow.

@lancewillett
Copy link
Contributor

@ebinnion API — OK, sounds good.

For the dates — is it really future-proof if it uses 2015 in the string? Will that work next winter, too, without a code change?

@ebinnion
Copy link
Contributor Author

ebinnion commented Dec 1, 2015

In regards to the dates, it should be future proof.

Explanation:

let startDate = this.moment( '2015-11-30' ).year( thisYear ),
    endDate = this.moment( '2015-01-05' ).year( thisYear + 1 );

To create a moment, we need to pass it a proper string (otherwise I was getting deprecation notices) that looks like 2015-11-30.

So, this.moment( '2015-11-30' ) creates the moment. Then .year( thisYear ) updates the year of that moment we just created to match that of the current year. I really just needed a string that had the correct month and date since I was going to force the year.

@lancewillett
Copy link
Contributor

Got it! Thanks.

Testing things out — looks great. 🚢

ebinnion added a commit that referenced this pull request Dec 1, 2015
Me: Adds ability to toggle holiday snow
@ebinnion ebinnion merged commit 41fcc4d into master Dec 1, 2015
@ebinnion ebinnion deleted the update/me-holiday-snow branch December 1, 2015 22:50
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Dec 1, 2015
@Viper007Bond
Copy link
Contributor

What happens on January 2nd, 2016 for example? Unless I'm overlooking something, your code won't work. startDate will be be in the future now (the upcoming November) and endDate will be 2017-01-05 rather than a few days from now. I think your code's logic will make it disappear early, on New Year's.

The correct logic would be something like this:

let startDate = this.moment( '2015-12-1' ).year( thisYear ),
    endDate = this.moment( '2015-01-04' ).year( thisYear );

if ( today < startDate && today > endDate ) {
    return;
}

Note: I have no idea how moment() works and if it's making a timestamp or what so my if statement is likely bogus, I was just trying to demonstrate the correct logic.

In short, if today is after this year's January 4th and before this year's December 1st, don't display it.

@Viper007Bond
Copy link
Contributor

Additionally perhaps we should control this on the REST API side of things so that if we decide to change either of the dates in the PHP code, the change is reflected in Calypso as well. There's talk of extending it to the 6th for example.

@ebinnion
Copy link
Contributor Author

ebinnion commented Dec 2, 2015

Thanks for pointing that out @Viper007Bond. So, to clarify, the solution would have worked for future years, it just would've hid the checkbox early when January came around.

In regards to relying on the API for the start and end date, those dates are also hard coded in Jetpack. Since those dates are hard coded in Jetpack as well, I think we should move forward with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] User & Account Settings (/me) Settings and tools for managing your WordPress.com user account.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants