-
Notifications
You must be signed in to change notification settings - Fork 815
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
Search: Add pricing and tier information to Plans page #15125
Search: Add pricing and tier information to Plans page #15125
Conversation
@keoshi: Due to the slower response times for the Tiers API, I split out the loading states for Backups and Search. Unfortunately, that means this is now possible: Should I unify the loading states for Backups and Search so that we could avoid this misalignment? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor copy/design comments...
e68eef7
to
33e1b3b
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good to me (with the caveat that I didn't actually test it). However, I think there's one thing that may be broken -- see inline comment.
I don't if there's prior art in doing something like this or a best practice in terms of performance, but I'd personally much rather see them show up at the same time, yes! :) |
This is looking good to me, I think we are waiting for the wp.com deploy anyways, so will hold off on a final review until that lands. |
@@ -256,11 +256,14 @@ export default connect( state => { | |||
multisite: isMultisite( state ), | |||
products: getProducts( state ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in one of the earlier comments you mentioned this component got slower to load (due to the extra API endpoint)...
I think you can actually eliminate this products
property from the component (and the isFetchingProducts
property below) and just pass siteProducts
in to SingleProductBackup
instead -- this should work fine since the two have the same structure (and for Jetpack Backup, the exact same data).
In that case, you can probably also avoid calling QueryProducts
on this page entirely, which would avoid having to hit that API endpoint at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested this along with D40938 and it seems to all work well. The redirects are currently not working because we need to move the search plans to staging, but they all look correct based on inspection.
* Search: Add Search plan to wp-admin Plans/My-Plans pages (#15011) * Add Instant Search feature gate * Add Jetpack Search to the Plans page * Instant Search: Add support in glance and performance sections (#15043) * Search: Add instant search auto config (#15026) * Significant refactoring of Plans pages, especially for Backups * Update the copy for search module. (#15123) * Search: Add search plan to my-plans page (#15095) * Search: Add pricing and tier information to Plans page (#15125) * Search: Fix search plan detection (#15156)
Jetpack equivalent to Automattic/wp-calypso#40306.
Changes proposed in this Pull Request:
/sites/<id>/products
API route via<QuerySiteProducts />
,site-products
state handling, andjetpack/v4/site/products
local API route.Is this a new feature or does it add/remove features to an existing part of Jetpack?
Testing instructions:
You must apply Phabricator diff 153244 to your sandbox. This is a variant of D38944-code.
Apply changes from this PR to your Jetpack installation. If you're running a dockerized instance, you may need to update your
extra_hosts
to properly sandbox the WPCOM API.Navigate to the Jetpack Plans page at
/wp-admin/admin.php?page=jetpack#/plans
.Ensure that both the record counts and the tier information render as expected.
Proposed changelog entry for your changes: