364 add the integration card for google site kit #21946
Conversation
add card
fix lint
fix semicolon
fix typo
vraja-pro
added
the
changelog: non-user-facing
And renaming tests links
fix tests to run in separate process. This is done because the conditional foe jetpack and elementor returns different results when running the test separately.
vraja-pro
force-pushed
the
364-add-the-integration-card-for-google-site-kit
branch
from
c304683 to
4c3bbdd
Compare
Pull Request Test Coverage Report for Build 5027634b5b73f99fb989cd278943550041725a0aDetails
💛 - Coveralls |
There was a problem hiding this comment.
CR 🏗️
About the changelog entries
Removes "New" badges from the integration page.
That is for sure user-facing. Whether that is worth mentioning is another. But I don't see any decisions, so I mention it.
Enables translation for the "Learn more" in the integration cards.
Isn't that a bugfix? E.g. something like
- Fixes a bug where
Learn moreon the integrations page would not be translated.
Adds unreleased integration card for Google Site Kit.
Unreleased? You mean behind feature flag.
| global.wpseoScriptData = { | ||
| isPremium: 0, | ||
| }; | ||
|
|
There was a problem hiding this comment.
Why would you add this for all tests?
| install: sprintf( | ||
| /* translators: 1: Site Kit by Google */ | ||
| __( "Install %1$s", "wordpress-seo" ), | ||
| "Site Kit by Google" |
There was a problem hiding this comment.
They do translate Site Kit: https://github.com/google/site-kit-wp/blob/develop/includes/Core/Admin/Screen.php#L129
And Site Kit by Google: https://github.com/google/site-kit-wp/blob/develop/includes/Core/Admin/Screen.php#L87
| * @returns {WPElement} The Site Kit integration component. | ||
| */ | ||
| export const GoogleSiteKitIntegration = () => { | ||
| const { isActive, afterSetup, isInstalled, isConnected } = get( window, "wpseoIntegrationsData.googleSiteKit", { isActive: false, afterSetup: false, isInstalled: false, isConnected: false } ); |
There was a problem hiding this comment.
I think you should not read the window state inside this component.
Instead, take an example of the Woo integration and use props to connect it from the recommended plugins in this case.
That would've made your tests also more straight forward to use normal props for component rendering.
| const { isActive, afterSetup, isInstalled, isConnected } = get( window, "wpseoIntegrationsData.googleSiteKit", { isActive: false, afterSetup: false, isInstalled: false, isConnected: false } ); | ||
| const [ isModalOpen, toggleModal ] = useToggleState( false ); | ||
|
|
||
| const getButtonConfig = useCallback( () => { |
There was a problem hiding this comment.
Instead of config. These are the props, right?
E.g. getButtonProps
| <> | ||
| <SimpleIntegration | ||
| integration={ integration } | ||
| isActive={ isInstalled && isActive } |
There was a problem hiding this comment.
| isActive={ isInstalled && isActive } | |
| isActive={ isInstalled && isActive } |
|
|
||
| if ( ! isInstalled ) { | ||
| button.children = buttonLabels.install; | ||
| button.href = "/wp-admin/plugin-install.php?s=google%2520site%2520kit&tab=search&type=term"; |
There was a problem hiding this comment.
This and below, the admin URLs should be retrieved from PHP to ensure compatibility for changes. See the integration' use of self_admin_url
| id: "google-site-kit-button", | ||
| }; | ||
|
|
||
| if ( ! isInstalled ) { |
There was a problem hiding this comment.
This giant if/else is a bit bewildering.
Later on you combine states as well. I wonder if this can be done in an easier way.
Really, you have just a button in the content and a status in the footer.
| logo: SiteKitLogo, | ||
| }; | ||
|
|
||
| const buttonLabels = { |
There was a problem hiding this comment.
Why only labels and not also the rest of the button props?
| const notConnected = useMemo( () => ! isConnected || ! afterSetup, [ isConnected, afterSetup ] ); | ||
| const pluginNotDetected = useMemo( () => ! isActive || ! isInstalled, [ isActive, isInstalled ] ); | ||
| const successfullyConnected = useMemo( () => isActive && afterSetup && isConnected, [ isActive, afterSetup, isConnected ] ); |
There was a problem hiding this comment.
These useMemo usages seem a bit overkill for simple boolean logic
| import { GoogleSiteKitIntegration } from "../../src/integrations-page/google-site-kit-integration"; | ||
| import { expect } from "@jest/globals"; | ||
|
|
||
| describe( "GoogleSiteKitIntegration", () => { |
There was a problem hiding this comment.
These are the happy paths.
I wonder, with all that logic if/else, what would happen if you brute force all the combinations 😁
Context
Summary
This PR can be summarized in the following changelog entry:
Relevant technical choices:
Test instructions
Test instructions for the acceptance test before the PR gets merged
This PR can be acceptance tested by following these steps:
Relevant test scenarios
Test instructions for QA when the code is in the RC
QA can test this PR by following these steps:
Impact check
This PR affects the following parts of the plugin, which may require extra testing:
UI changes
Other environments
[shopify-seo], added test instructions for Shopify and attached theShopifylabel to this PR.Documentation
Quality assurance
Innovation
innovationlabel.Fixes Figure out how we can detect if Site Kit is configured and add the integration card