Site Settings: Use real-time activation state of SEO and Verification modules #11669
Conversation
lancewillett
added
the
[Status] Needs Review
There was a problem hiding this comment.
Works as described. I've posted a couple of small suggestions.
Also one thing that bothers me is that if any of those modules are disabled, we're not disabling all of the related fields and buttons. For example, if the SEO Tools module is disabled, we must disable all of the associated fields and buttons in that card. We're doing that for many other Jetpack cards throughout site settings (Subscriptions, Theme Enhancements, Custom Content Types, Publishing Tools).
| const jetpackManagementUrl = getSiteOption( state, siteId, 'admin_url' ); | ||
| const jetpackVersionSupportsSeo = isJetpackMinimumVersion( state, siteId, '4.4-beta1' ); | ||
| const isAdvancedSeoSupported = site && ( ! site.jetpack || ( site.jetpack && jetpackVersionSupportsSeo ) ); | ||
|
|
||
| return { | ||
| siteId: siteId, |
There was a problem hiding this comment.
You can use the object literal property value shorthand here, which allows you to change this line to:
siteId,
| @@ -437,6 +442,10 @@ export const SeoForm = React.createClass( { | |||
| /* eslint-disable react/jsx-no-target-blank */ | |||
| return ( | |||
| <div> | |||
| { | |||
| site.jetpack && | |||
There was a problem hiding this comment.
Let's use the isJetpackSite selector here instead of site.jetpack.
tyxla
added
[Status] Needs Author Reply
and removed
[Status] Needs Review
matticbot
added
[Size] M
|
@tyxla thanks for the suggestions, I've updated the code accordingly. |
vindl
force-pushed
the
fix/settings-seo-module-active-state
branch
from
3e7b41e to
20e36a3
Compare
matticbot
added
[Size] M
vindl
added
[Status] Needs Review
|
@vindl This PR needs a rebase |
| const isDisabled = isSitePrivate || isJetpackUnsupported || isSubmittingForm || isFetchingSettings; | ||
| const isSeoDisabled = isDisabled || ( isSeoToolsActive === false ); | ||
| const isVerificationDisabled = isDisabled || ( isVerificationToolsActive === false ); |
There was a problem hiding this comment.
Do we really need parentheses on these 2 lines?
There was a problem hiding this comment.
Not really, it's just something that I've added to make it easier to scan (more readable). Updated.
| { submitButton } | ||
| <SectionHeader label={ translate( 'Site Verification Services' ) }> | ||
| <Button | ||
| compact={ true } |
There was a problem hiding this comment.
compact={ true } can be shortened to just compact
| <Button | ||
| compact={ true } | ||
| onClick={ this.submitSeoForm } | ||
| primary={ true } |
There was a problem hiding this comment.
primary={ true } can be shortened to just primary
vindl
force-pushed
the
fix/settings-seo-module-active-state
branch
from
4b0f6bd to
2c8163c
Compare
vindl
force-pushed
the
fix/settings-seo-module-active-state
branch
from
2c8163c to
8f08773
Compare
vindl
force-pushed
the
fix/settings-seo-module-active-state
branch
from
8f08773 to
2795f3e
Compare
vindl
removed
[Status] Needs Rebase
[Status] Needs Review
This fixes a bug with Jetpack sites where SEO Tools and Site Verification modules can appear as disabled in Calypso if the module has been activated recently, and the sync hasn't completed yet.
To reproduce the bug:
/settings/seo/{siteSlug}wp-admin/admin.php?page=jetpack#/engagementof your test Jetpack site/settings/seo/{siteSlug}and see that the module still appears to be inactive.Testing instructions:
wp-admin/admin.php?page=jetpack#/engagementof your test Jetpack site with a Professional plan./settings/seo/{siteSlug}and verify thatmodule disablednotices are shown for these modules.wp-admin/admin.php?page=jetpack#/engagementactivate SEO Tools and Site Verification modules./settings/seo/{siteSlug}and verify thatmodule disablednotices are no longer being shown.