-
Notifications
You must be signed in to change notification settings - Fork 898
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
364 add the integration card for google site kit #21946
base: trunk
Are you sure you want to change the base?
Changes from 15 commits
3d0ba21
a1918eb
cf89cd7
ac117dc
54f73a6
30c5303
efd5482
b052c0b
6ffb2ba
38c1090
65a6732
4230698
be33a54
657eccc
4c3bbdd
5a78940
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,139 @@ | ||||||
import { __, sprintf } from "@wordpress/i18n"; | ||||||
import { CheckIcon, XIcon } from "@heroicons/react/solid"; | ||||||
import { Fragment, createInterpolateElement, useMemo, useCallback } from "@wordpress/element"; | ||||||
import { SimpleIntegration } from "./simple-integration"; | ||||||
import { ReactComponent as SiteKitLogo } from "../../images/site-kit-logo.svg"; | ||||||
import { get } from "lodash"; | ||||||
import { useToggleState } from "@yoast/ui-library"; | ||||||
import { SiteKitConsentModal } from "../shared-admin/components"; | ||||||
|
||||||
const integration = { | ||||||
name: "Site Kit by Google", | ||||||
claim: createInterpolateElement( | ||||||
sprintf( | ||||||
/* translators: 1: bold open tag; 2: Site Kit by Google; 3: bold close tag. */ | ||||||
__( "Get valuable insights with %1$s%2$s%3$s", "wordpress-seo" ), | ||||||
"<strong>", | ||||||
"Site Kit by Google", | ||||||
"</strong>" | ||||||
), { | ||||||
strong: <strong />, | ||||||
} | ||||||
), | ||||||
learnMoreLink: "https://yoa.st/integrations-google-site-kit", | ||||||
logoLink: "https://yoa.st/integrations-google-site-kit", | ||||||
slug: "google-site-kit", | ||||||
description: __( "View traffic and search rankings on your dashboard by connecting your Google account.", "wordpress-seo" ), | ||||||
isPremium: false, | ||||||
isNew: false, | ||||||
isMultisiteAvailable: false, | ||||||
logo: SiteKitLogo, | ||||||
}; | ||||||
|
||||||
const buttonLabels = { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why only labels and not also the rest of the button props? |
||||||
install: sprintf( | ||||||
/* translators: 1: Site Kit by Google */ | ||||||
__( "Install %1$s", "wordpress-seo" ), | ||||||
"Site Kit by Google" | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They do translate |
||||||
), | ||||||
activate: sprintf( | ||||||
/* translators: 1: Site Kit by Google */ | ||||||
__( "Activate %1$s", "wordpress-seo" ), | ||||||
"Site Kit by Google" | ||||||
), | ||||||
setup: sprintf( | ||||||
/* translators: 1: Site Kit by Google */ | ||||||
__( "Set up %1$s", "wordpress-seo" ), | ||||||
"Site Kit by Google" | ||||||
), | ||||||
connect: sprintf( | ||||||
/* translators: 1: Site Kit by Google */ | ||||||
__( "Connect %1$s", "wordpress-seo" ), | ||||||
"Site Kit by Google" | ||||||
), | ||||||
disconnect: __( "Disconnect", "wordpress-seo" ), | ||||||
}; | ||||||
|
||||||
/** | ||||||
* The Site Kit integration component. | ||||||
* | ||||||
* @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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you should not read the window state inside this component. That would've made your tests also more straight forward to use normal props for component rendering. |
||||||
const [ isModalOpen, toggleModal ] = useToggleState( false ); | ||||||
|
||||||
const getButtonConfig = useCallback( () => { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of config. These are the props, right? E.g. |
||||||
const button = { | ||||||
className: "yst-mt-6 yst-w-full", | ||||||
as: "a", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Weird that the |
||||||
id: "google-site-kit-button", | ||||||
}; | ||||||
|
||||||
if ( ! isInstalled ) { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This and below, the admin URLs should be retrieved from PHP to ensure compatibility for changes. See the integration' use of |
||||||
} else if ( ! isActive ) { | ||||||
button.children = buttonLabels.activate; | ||||||
button.href = "/wp-admin/plugins.php"; | ||||||
} else if ( ! afterSetup ) { | ||||||
button.children = buttonLabels.setup; | ||||||
button.href = "/wp-admin/admin.php?page=googlesitekit-splash"; | ||||||
} else if ( ! isConnected ) { | ||||||
button.children = buttonLabels.connect; | ||||||
button.onClick = toggleModal; | ||||||
button.as = "button"; | ||||||
} else if ( isConnected ) { | ||||||
button.children = buttonLabels.disconnect; | ||||||
button.as = "button"; | ||||||
button.variant = "secondary"; | ||||||
} | ||||||
|
||||||
return button; | ||||||
}, [ isInstalled, isActive, afterSetup, isConnected ] ); | ||||||
|
||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These |
||||||
|
||||||
return ( | ||||||
<> | ||||||
<SimpleIntegration | ||||||
integration={ integration } | ||||||
isActive={ isInstalled && isActive } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
button={ getButtonConfig( isInstalled, isActive, afterSetup, isConnected ) } | ||||||
> | ||||||
|
||||||
{ successfullyConnected && <Fragment> | ||||||
<span className="yst-text-slate-700 yst-font-medium">{ __( "Successfully connected", "wordpress-seo" ) }</span> | ||||||
<CheckIcon | ||||||
className="yst-h-5 yst-w-5 yst-text-green-400 yst-flex-shrink-0" | ||||||
/> | ||||||
</Fragment> } | ||||||
|
||||||
{ notConnected && isActive && <Fragment> | ||||||
<span className="yst-text-slate-700 yst-font-medium"> | ||||||
{ | ||||||
__( "Not connected", "wordpress-seo" ) | ||||||
} | ||||||
</span> | ||||||
<XIcon | ||||||
className="yst-h-5 yst-w-5 yst-text-red-500 yst-flex-shrink-0" | ||||||
/> | ||||||
</Fragment> } | ||||||
|
||||||
{ pluginNotDetected && <Fragment> | ||||||
<span className="yst-text-slate-700 yst-font-medium"> | ||||||
{ | ||||||
__( "Plugin not detected", "wordpress-seo" ) | ||||||
} | ||||||
</span> | ||||||
<XIcon | ||||||
className="yst-h-5 yst-w-5 yst-text-red-500 yst-flex-shrink-0" | ||||||
/> | ||||||
</Fragment> } | ||||||
</SimpleIntegration> | ||||||
<SiteKitConsentModal isOpen={ isModalOpen } onClose={ toggleModal } /> | ||||||
</> | ||||||
); | ||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,10 +13,11 @@ import { getIsFreeIntegrationOrPremiumAvailable } from "./helper"; | |
* @param {object} integration The integration. | ||
* @param {boolean} isActive The integration state. | ||
* @param {wp.Element} children The child components. | ||
* @param {wp.Element} button The button component. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These are actually just button props, of type object. Not an element like this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right! I was first thinking to pass the element and then decided to pass the props... |
||
* | ||
* @returns {WPElement} A card representing an integration. | ||
*/ | ||
export const SimpleIntegration = ( { integration, isActive, children } ) => { | ||
export const SimpleIntegration = ( { integration, isActive, children, button } ) => { | ||
const IntegrationLogo = integration.logo; | ||
|
||
return ( | ||
|
@@ -59,7 +60,7 @@ export const SimpleIntegration = ( { integration, isActive, children } ) => { | |
className="yst-flex yst-items-center yst-mt-3 yst-no-underline yst-font-medium" | ||
target="_blank" | ||
> | ||
Learn more | ||
{ __( "Learn more", "wordpress-seo" ) } | ||
<span className="yst-sr-only"> | ||
{ | ||
/* translators: Hidden accessibility text. */ | ||
|
@@ -68,11 +69,13 @@ export const SimpleIntegration = ( { integration, isActive, children } ) => { | |
</span> | ||
<ArrowSmRightIcon className="yst-h-4 yst-w-4 yst-ms-1 yst-icon-rtl" /> | ||
</Link> } | ||
|
||
{ button && <Button { ...button } /> } | ||
Comment on lines
+72
to
+73
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Getting less simple by the commit 🙈 |
||
</div> | ||
</Card.Content> | ||
<Card.Footer> | ||
{ ! getIsFreeIntegrationOrPremiumAvailable( integration ) && <Button | ||
id={ `${ integration.name }-upsell-button` } | ||
id={ `${ integration.slug }-upsell-button` } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Might this have consequences for automated tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, got an approval from Natalia. |
||
type="button" | ||
as="a" | ||
href={ integration.upsellLink } | ||
|
@@ -121,9 +124,11 @@ SimpleIntegration.propTypes = { | |
PropTypes.node, | ||
PropTypes.arrayOf( PropTypes.node ), | ||
] ), | ||
button: PropTypes.object, | ||
}; | ||
|
||
SimpleIntegration.defaultProps = { | ||
isActive: true, | ||
children: [], | ||
button: null, | ||
}; |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You expect this to be reused in the dashboard? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
import { Modal } from "@yoast/ui-library"; | ||
import { __ } from "@wordpress/i18n"; | ||
import { PropTypes } from "prop-types"; | ||
|
||
/** | ||
* The Site Kit consent modal component. | ||
* | ||
* @param {boolean} isOpen Whether the modal is open. | ||
* @param {Function} onClose Callback to close the modal. | ||
* | ||
* @returns {WPElement} The Site Kit consent modal component. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We normally type this as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, my mistake! |
||
*/ | ||
export const SiteKitConsentModal = ( { isOpen, onClose } ) => { | ||
return ( | ||
<Modal | ||
isOpen={ isOpen } | ||
onClose={ onClose } | ||
> | ||
<Modal.Panel> | ||
<Modal.Title>{ __( "Connect Site Kit by Google", "wordpress-seo" ) }</Modal.Title> | ||
<Modal.Description> | ||
<p>{ __( "Connect your Google account to view traffic and search rankings on your dashboard.", "wordpress-seo" ) }</p> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
</Modal.Description> | ||
</Modal.Panel> | ||
</Modal> | ||
); | ||
}; | ||
|
||
SiteKitConsentModal.propTypes = { | ||
isOpen: PropTypes.bool, | ||
onClose: PropTypes.func, | ||
}; |
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.
Not translated?
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.
None of the cards names are translated, so I guess we should translate all the rest (or some)?