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.

Sign up for GitHub

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

Jump to bottom

Domains: show info notice when user has domain credit #4881

Merged
merged 1 commit into from
Apr 20, 2016
Merged

Domains: show info notice when user has domain credit #4881

merged 1 commit into from
Apr 20, 2016

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Apr 19, 2016 •
edited
Loading

This PR adds an information notice to domains, when a user has purchased a plan upgrade but hasn't yet claimed their custom domain. Part of #4858
reminder

Testing Instructions

  • Turn on analytics debug: localStorage.setItem('debug', 'calypso:analytics');
  • Upgrade a free site to premium or business
  • Navigate to http://calypso.localhost:3000/domains and select your upgraded site
  • Set the testing variant to: localStorage.setItem('ABTests','{"domainCreditsInfoNotice_20160420":"showNotice"}')
  • Your upgraded site should see the notice, and other sites should not
  • The following analytics events should fire:
    impression
    click
  • Set the testing variant to:
    localStorage.setItem('ABTests','{"domainCreditsInfoNotice_20160420":"original"}')
  • Navigate to http://calypso.localhost:3000/domains and select your upgraded site
  • Notice does not display, but the following analytics event fires:
    screen shot 2016-04-20 at 8 33 20 am

cc @rralian @mtias @dmsnell @artpi @retrofox @adambbecker

@gwwar gwwar self-assigned this Apr 19, 2016
@gwwar gwwar added [Status] In Progress [Feature Group] Emails & Domains Features related to email integrations and domain management. labels Apr 19, 2016
@gwwar gwwar added this to the Plans Iteration 1 milestone Apr 19, 2016
@gwwar gwwar force-pushed the add/domain-credit-notice branch from d49b005 to 42f1750 Compare April 19, 2016 23:29
retrofox
retrofox reviewed Apr 19, 2016
View reviewed changes
client/my-sites/upgrades/domain-management/list/index.jsx
import { hasDomainCredit } from 'state/sites/plans/selectors';
import TrackComponentView from 'lib/analytics/track-component-view';
import {
recordTracksEvent
Copy link
Contributor

Choose a reason for hiding this comment

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

why line breaks here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, I originally imported a few more actions.

@gwwar gwwar force-pushed the add/domain-credit-notice branch 2 times, most recently from 3e154c4 to 23f8c14 Compare April 19, 2016 23:41
retrofox
retrofox reviewed Apr 19, 2016
View reviewed changes
client/state/sites/plans/selectors.js
const currentPlan = find( plans.data, 'currentPlan' );
return currentPlan.hasDomainCredit;
}
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it isn't needed return null;. We could remove this line here.

Copy link
Contributor Author

@gwwar gwwar Apr 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

In selectors we tend to return null when we have an invalid/loading state

@gwwar gwwar 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 Apr 19, 2016
mtias
mtias reviewed Apr 20, 2016
View reviewed changes
client/my-sites/upgrades/domain-management/list/index.jsx
@@ -67,6 +76,19 @@ const List = React.createClass( {
sitePlans={ this.props.sitePlans } />
{ this.domainWarnings() }

{ this.props.hasDomainCredit && <Notice
Copy link
Member

Choose a reason for hiding this comment

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

This is silly thing, but I prefer to have the beginning of the component element in a new line for visual scanning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated in 05585f6

@mtias
Copy link
Member

mtias commented Apr 20, 2016

Looking good to me, I'll pick up the work on the selectors for my nudge in current-site once it's merged.

@rralian
Copy link
Contributor

rralian commented Apr 20, 2016

I'd like to be able to track the impact of this. Can we run a 90/10 test, where we display the reminder 90% of the time (as appropriate) and we don't display it 10% of the time, but we trigger an impression (or no-impression) event to track both cases, and then we can run both variants against refund numbers? This wouldn't be so much to determine whether this is a good thing to do (I think it's pretty clearly a positive), but rather to track the impact of the effort.

@gwwar gwwar force-pushed the add/domain-credit-notice branch from 23f8c14 to 05585f6 Compare April 20, 2016 15:00
@gwwar
Copy link
Contributor Author

gwwar commented Apr 20, 2016 •
edited
Loading

Can we run a 90/10 test

I'll add that in shortly. Edit: Updated in 78a1c88

dmsnell
dmsnell reviewed Apr 20, 2016
View reviewed changes
client/my-sites/upgrades/domain-management/list/index.jsx
};
}, ( dispatch ) => {
return {
clickClaimDomainNotice: () => dispatch( recordTracksEvent(
Copy link
Member

Choose a reason for hiding this comment

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

nice!

@rralian
Copy link
Contributor

rralian commented Apr 20, 2016

Works great! :shipit:

@rralian rralian 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 Apr 20, 2016
@gwwar
Copy link
Contributor Author

gwwar commented Apr 20, 2016

Thanks for the reviews! Going to squash and rebase

@gwwar gwwar force-pushed the add/domain-credit-notice branch from 4c31d96 to 90df60b Compare April 20, 2016 23:27
@gwwar gwwar force-pushed the add/domain-credit-notice branch from 90df60b to c1c41f8 Compare April 20, 2016 23:30
@gwwar gwwar merged commit f65533a into master Apr 20, 2016
@gwwar gwwar deleted the add/domain-credit-notice branch April 20, 2016 23:37
cadecairos pushed a commit to mozilla/wp-calypso that referenced this pull request Apr 27, 2016
@gwwar
Domains: show info notice when user has domain credit (Automattic#4881)
2123fa8
@fabianapsimoes fabianapsimoes mentioned this pull request Apr 28, 2016
Closed
@umurkontaci umurkontaci mentioned this pull request May 11, 2016
Merged
cadecairos pushed a commit to mozilla/wp-calypso that referenced this pull request May 26, 2016
@gwwar
Domains: show info notice when user has domain credit (Automattic#4881)
6247ba9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@gwwar gwwar

Labels
[Feature Group] Emails & Domains Features related to email integrations and domain management.
Projects
None yet
Milestone
Plans: Iteration 1
Development

Successfully merging this pull request may close these issues.
6 participants
@gwwar @mtias @rralian @retrofox @dmsnell @scruffian