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

Sites: Don't bind selectors to state #14030

Merged
merged 4 commits into from
May 24, 2017
Merged

Conversation

mcsf
Copy link
Member

@mcsf mcsf commented May 14, 2017

Part of #14024

This PR seeks to remove

export default connect(
  ( state ) => ( {
    isSiteUpgradeable: isSiteUpgradeable.bind( null, state ),
  } )
)( Sites );

from my-sites/sites. @tyxla: You've been working a lot with Jetpack and capabilities, do you have recommendations on how to approach this? Sites passes a filter function prop to SiteSelector, which calls it to filter all site objects. How wrong would it be to inspect the site objects instead of calling the selector?

@matticbot
Copy link
Contributor

@tyxla
Copy link
Member

tyxla commented May 16, 2017

@mcsf I'm thinking we can add isUpgradeable as a computed prop. During the sites-list migration we've been trying to keep the computed props as less as possible, so I think we can add this one. We're already doing that for is_previewable anyway.

@mcsf
Copy link
Member Author

mcsf commented May 17, 2017

@tyxla: sounds like a good idea. Would you be interested in picking it up, or should I?

@tyxla
Copy link
Member

tyxla commented May 18, 2017

Sure, @mcsf, I've added a couple of commits to tackle that 👍

@mcsf
Copy link
Member Author

mcsf commented May 18, 2017

Woot! Thanks!

@mcsf mcsf 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 May 18, 2017
@mcsf
Copy link
Member Author

mcsf commented May 18, 2017

I can't formally approve this PR, since I created it, but LGTM!. :)

@tyxla tyxla requested review from ockham, ehg, mtias and gwwar May 19, 2017 09:22
@tyxla
Copy link
Member

tyxla commented May 19, 2017

Well, let's request some more 👀 here then 😉

Copy link
Contributor

@gwwar gwwar left a comment

Choose a reason for hiding this comment

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

I'm no longer able to search for wpcom sites that have upgrades, or can be upgraded. For example when visiting http://calypso.localhost:3000/plans

@tyxla
Copy link
Member

tyxla commented May 22, 2017

@gwwar good catch there!

In 1555eff I've applied a fix for that will allow to also include sites for which we can't determine whether they're upgradeable. Feel free to give the PR another try. Thanks!

@@ -47,7 +47,7 @@ export const Sites = React.createClass( {

// Filter out sites with no upgrades on particular routes
if ( /^\/domains/.test( path ) || /^\/plans/.test( this.props.sourcePath ) ) {
return this.props.isSiteUpgradeable( site.ID );
return site.isSiteUpgradeable !== false;
Copy link
Contributor

Choose a reason for hiding this comment

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

This does the trick, but I wonder if we could be a bit more clear with:

return ! site.isJetpack || site.isSiteUpgradable;

Copy link
Member

Choose a reason for hiding this comment

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

Sure, I'd go that way, but I was wondering: is there a case where upgradeable state can't be determined for a WordPress.com site? For example, if a site request fails to complete.

Copy link
Contributor

Choose a reason for hiding this comment

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

What does state look like in that case? I'd also be fine with the above and adding a clarifying comment so folks don't try and simplify the check.

Copy link
Member

Choose a reason for hiding this comment

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

After further consideration, I've decided to go with your suggestion here, it makes it much more readable than what I suggested.

@tyxla
Copy link
Member

tyxla commented May 24, 2017

@mcsf that one is ready to go; do you want to deploy it, or should I? 😉

@tyxla tyxla 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 May 24, 2017
@mcsf
Copy link
Member Author

mcsf commented May 24, 2017

@tyxla I feel you should do the honors, since I merely opened the PR and you did all the work :)

@tyxla tyxla force-pushed the remove/sites-selector-state-binding branch from 40c5c11 to 9510156 Compare May 24, 2017 12:19
@tyxla tyxla merged commit 789ac3e into master May 24, 2017
@tyxla tyxla deleted the remove/sites-selector-state-binding branch May 24, 2017 12:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants