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

Pressable: show proper my-plan page #8847

Merged
merged 4 commits into from
Oct 27, 2016
Merged

Conversation

artpi
Copy link
Contributor

@artpi artpi commented Oct 19, 2016

This is a part of #8836.

  • Introduces state.sites.ID.options.pressable option in sites redux tree
  • Introduces isPressableSite selector
  • Introduces PRESSABLE_TRANSFER_SUCCESS action to test pressable internals
  • Disables domains manage link in my-plan page

Testing

  • Go to my-plan page for a business site
  • Open redux dispatcher from redux devtools
  • Dispatch { type: 'PRESSABLE_TRANSFER_SUCCESS', siteId: ID }
  • See that button disappears for managing domain OR whole call to action disappears for using domain credit

CC @rralian @gwwar @lamosty @retrofox @adambbecker

@artpi artpi added [Status] In Progress [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Oct 19, 2016
@artpi artpi added this to the Plans: Maintenance milestone Oct 19, 2016
@artpi artpi self-assigned this Oct 19, 2016
@matticbot
Copy link
Contributor

@rralian
Copy link
Contributor

rralian commented Oct 19, 2016

FAQ

EDIT: @artpi - I moved the questions here from the thread main post:

  • How are you going to use your domain credit while on this plan? Do we implement domain management?
  • Should we show "Advertising removed" section? Does not make sense for jetpack, but its still continuing features from previous plans
  • Is the .com theme catalogue going to be hooked up ? For jetpack sites we currently show only installed themes
  • What about wordads instant activation? Is that going to be present in pressable?

How are you going to use your domain credit while on this plan? Do we implement domain management?

I've been discussing this a bit with @p3ob7o. No promises, but we may be able to enable domain management for these sites. In practice most users will not have a domain credit because automated-transfer will be limited to users with a custom domain. But it's possible for a business site to have a custom domain and still have an available domain credit, so we still need to work this out.

Should we show "Advertising removed" section? Does not make sense for jetpack, but its still continuing features from previous plans

Yes. These are dotcom business site users who have made use of the custom theme/plugin functionality. Jetpack is an implementation detail that isn't relevant to the user.

Is the .com theme catalogue going to be hooked up ? For jetpack sites we currently show only installed themes

Yes, that is the plan. See the spreadsheet which links back to the master thread for that effort.

What about wordads instant activation? Is that going to be present in pressable?

It should, yes. There's work involved there too though.

selectedSite={ selectedSite }
hasDomainCredit={ hasDomainCredit }
/>
<CustomDomainPurchaseDetail { ...pick( props, [ 'selectedSite', 'hasDomainCredit', 'isPressableSite' ] ) } />
Copy link
Contributor

Choose a reason for hiding this comment

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

I actually think the former is more readable but ¯_(ツ)_/¯

Copy link
Contributor

Choose a reason for hiding this comment

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

Me too. I think we should pass all the props (with picking/omitting some) only if they are passed to a native component like div or button. Those component can accept various different props so it's better to simply pass down everything instead of listing the possible props.

However, in this case, we are passing down only three props so it's unnecessary to use this pattern IMO. :)

Copy link
Contributor Author

@artpi artpi Oct 26, 2016

Choose a reason for hiding this comment

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

For each of these props it was repeating each prop name ( selectedSite for example) 3 times just to pass it down.
Each of these repeats compounds the likelihood of making an error when adding/removing/modfing passed prop.
I refactored it not because of readability, but because of DRY

return null;
}

return site.options.pressable;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Thanks for adding this. Let's check in with @peterbutler as work starts on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked in with Peter and he is completely ok with that. Looks like pressable plan will be just a regular business plan.
I think this can be a good selector to make work on this easier.
Worse case scenario, we can remove it later, but even having something to grep can be a good start

@artpi artpi force-pushed the experiment/pressable-my-plan branch from 83e53fb to 6ac97f6 Compare October 26, 2016 16:01
@gwwar gwwar added the [Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans. label Oct 26, 2016
const originalSite = state[ action.siteId ];
if ( originalSite ) {
return Object.assign( {}, state, {
[ action.siteId ]: merge( {}, originalSite, { jetpack: true, options: { pressable: true } } )
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a good start, but we'll probably also want to mark when a site is in the process of transferring.

<PurchaseDetail
);
} else if ( ! hasDomainCredit && hasCustomDomain( selectedSite ) ) {
const actionButton = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

The name here makes it sound like this is a component instead of a grouping of props. Perhaps we should refactor PurchaseDetail a bit.

Copy link
Contributor

Choose a reason for hiding this comment

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

A ternary might be a better fit, though the usage is still a bit odd.

const purchaseDetailButtonProps = isPressableSite ? {} : {
    buttonText: translate( 'Manage my domains' ),
    href: `/domains/manage/${ selectedSite.slug }`
};

return renderHasCustomDomain();
}

{ ...actionButton }
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it might make sense to update purchase detail to accept child components, and remove the PurchaseButton wrapper. (which only adds a class to button and is unnecessary ).

If this feels out of scope, lets mark this for another janitorial

@@ -75,6 +75,15 @@ export function items( state = {}, action ) {
}
return state;

case 'PRESSABLE_TEST':
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of this would more likely be something closer to PRESSIBLE_TRANSFER_START. Do you mind adding a new action type, even though we might rename this later.

@gwwar
Copy link
Contributor

gwwar commented Oct 26, 2016

I left a few notes, but I think we can merge this in after creating a proper action type.

@artpi
Copy link
Contributor Author

artpi commented Oct 27, 2016

After @gwwar pointers, I:

  • Introduced 2 selectors ( isPressableSite and isPressableSiteInTransfer )
  • Introduced 2 actions:PRESSABLE_TRANSFER_SUCCESS and PRESSABLE_TRANSFER_START

@artpi artpi merged commit e7bd8c6 into master Oct 27, 2016
@artpi artpi deleted the experiment/pressable-my-plan branch October 27, 2016 01:31
@lancewillett lancewillett removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 27, 2016
bisko pushed a commit that referenced this pull request Nov 16, 2016
Introduce 2 selectors ( isPressableSite and isPressableSiteInTransfer ) to easily
detect the state of pressable transfer and if site is in fact on pressable
Also hide "manage domain" button

* Add groundwork for marking site pressable
* Pass on pressable site status
* Introduce review pointers
* Make constants ssr-ready so test pass
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Plans & Upgrades All of the plans on WordPress.com and flow for upgrading plans.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants