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

JetpackFooter: add About, Privacy and Terms links #31627

Merged
merged 20 commits into from
Jul 20, 2023
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: minor
Type: added

Jetpack Footer: added generic links
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ const AdminPage: React.FC< AdminPageProps > = ( {
children,
moduleName = __( 'Jetpack', 'jetpack' ),
moduleNameHref,
a8cLogoHref,
showHeader = true,
showFooter = true,
showBackground = true,
Expand All @@ -43,11 +42,7 @@ const AdminPage: React.FC< AdminPageProps > = ( {
{ showFooter && (
<Container horizontalSpacing={ 5 }>
<Col>
<JetpackFooter
moduleName={ moduleName }
a8cLogoHref={ a8cLogoHref }
moduleNameHref={ moduleNameHref }
/>
<JetpackFooter moduleName={ moduleName } moduleNameHref={ moduleNameHref } />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

href of the Automattic logo is set in JetpackFooter directly.

</Col>
</Container>
) }
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,6 @@ export type AdminPageProps = {
*/
children: React.ReactNode;

/**
* Link for 'An Automattic Airline' in the footer.
*/
a8cLogoHref?: string;

/**
* Name of the module, e.g. 'Jetpack Search' that will be displayed in the footer.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,19 @@
Component that renders Jetpack Admin Footer.
It takes moduleName and URL to show in the footer.

#### How to use:
## How to use:

```js
<JetpackFooter
moduleName="Jetpack Search"
a8cLogoHref="https://www.jetpack.com"
className="jp-dashboard-footer"
/>
<JetpackFooter moduleName="Jetpack Search" className="jp-dashboard-footer" />
```

#### Props
## Props

- `className`: String - (default: `jp-dashboard-footer`) the additional class name set on the element.
- `a8cLogoHref`: String - (default: `https://www.jetpack.com`) link to be added on 'An Automattic Airline'.
- `moduleName`: String - (default: `Jetpack`) set the name of the Module, e.g. `Jetpack Search`.
- `menu`: JetpackFooterMenuItem[] - (default: `null`) set the menu items to be rendered in the footer.
- `moduleNameHref`: String - (default: `https://jetpack.com`) link that the Module name will link to.
- `menu`: JetpackFooterMenuItem[] - (default: `undefined`) set the menu items to be rendered in the footer.
- `onAboutClick`: () => void - (default: `undefined`) function called when the About link is clicked.
- `onPrivacyClick`: () => void - (default: `undefined`) function called when the Privacy link is clicked.
- `onTermsClick`: () => void - (default: `undefined`) function called when the Terms link is clicked.
- `siteAdminUrl`: String - (default: `undefined`) URL of the site WP Admin. Required to link to internal pages when applicable (e.g., Privacy).
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { __, _x } from '@wordpress/i18n';
import { Icon, external } from '@wordpress/icons';
import classnames from 'classnames';
import React from 'react';
import { getRedirectUrl } from '../..';
import { STORE_ID as CONNECTION_STORE_ID } from '../../../../js-packages/connection/state/store';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Importing from @automattic/jetpack-connection directly made the build fail because of some circular dependency. This import seems fine, but let me know if there's a better approach. Hardcoding the store id seemed error-prone.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this may prove problematic in the version of the package that we ship to npm.

Couldn't we bring in that data as a new component prop, to keep the component a bit simpler?

Hardcoding the store id seemed error-prone.

That'd be my next suggestion, with a comment in the original file mentioning that it's copied there, but I'm not a regular component maintainer so I'll let others weigh in on this. :)

Copy link
Member

Choose a reason for hiding this comment

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

Now that the connection package js is loaded externally and not bundled into any plugin after #38877, this import here can be problematic and result in duplicate store registration.

We should fix this in a better way. A few options we have:

  • Let the consumer component pass the needed connection related information
  • Make the component composable, which means the consumer component passes in the final output without this component here knowing anything about the connection data

Copy link
Member

@manzoorwanijk manzoorwanijk Sep 5, 2024

Choose a reason for hiding this comment

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

Another solution would be to use the data from the scrip-data package as explained in this thread.

Copy link
Member

Choose a reason for hiding this comment

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

That said, composability is still better because that will not make the component dependent upon anything related to jetpack connection or its logic.

import AutomatticBylineLogo from '../automattic-byline-logo';
import './style.scss';
import JetpackLogo from '../jetpack-logo';
import useBreakpointMatch from '../layout/use-breakpoint-match';
import type { JetpackFooterProps } from './types';
import type { JetpackFooterProps, JetpackFooterMenuItem } from './types';

const JetpackIcon: React.FC = () => (
<JetpackLogo logoColor="#000" showText={ false } height={ 16 } aria-hidden="true" />
Expand All @@ -19,17 +22,72 @@ const JetpackIcon: React.FC = () => (
* @returns {React.ReactNode} JetpackFooter component.
*/
const JetpackFooter: React.FC< JetpackFooterProps > = ( {
a8cLogoHref = 'https://automattic.com',
moduleName = __( 'Jetpack', 'jetpack' ),
className,
moduleNameHref = 'https://jetpack.com',
menu,
siteAdminUrl,
onAboutClick,
onPrivacyClick,
onTermsClick,
...otherProps
} ) => {
const [ isSm ] = useBreakpointMatch( 'sm', '<=' );
const [ isMd ] = useBreakpointMatch( 'md', '<=' );
const [ isLg ] = useBreakpointMatch( 'lg', '>' );

const { isActive, connectedPlugins } = useSelect(
select => {
const connectionStatus = select( CONNECTION_STORE_ID ) as {
getConnectedPlugins: () => { slug: string }[];
getConnectionStatus: () => { isActive: boolean };
};

return {
connectedPlugins: connectionStatus?.getConnectedPlugins(),
...connectionStatus.getConnectionStatus(),
};
},
[ CONNECTION_STORE_ID ]
);

const areAdminLinksEnabled =
siteAdminUrl &&
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ideally, siteAdminUrl would be retrieved from an initial state, as is connectedPlugins. I haven't found a way of retrieving basic site information that works across packages yet.

Copy link
Member

Choose a reason for hiding this comment

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

We now have getAdminUrl and other such utils available in @automattic/jetpack-script-data

import { getAdminUrl, getJetpackAdminPageUrl } from '@automattic/jetpack-script-data';

const aboutPageUrl = getAdminUrl( 'admin.php?page=jetpack_about' );

const privacyPageUrl = getJetpackAdminPageUrl( '#/privacy' );

Copy link
Member

Choose a reason for hiding this comment

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

Likewise, you can use the same package to get the list of connected plugins and the status.

import { getScriptData } from '@automattic/jetpack-script-data';

const { connectedPlugins, connectionStatus } = getScriptData().connection;

// Some admin pages require the site to be connected (e.g., Privacy)
isActive &&
// Admin pages are part of the Jetpack plugin and required it to be installed
connectedPlugins?.some( ( { slug } ) => 'jetpack' === slug );

let items: JetpackFooterMenuItem[] = [
{
label: _x( 'About', 'Link to learn more about Jetpack.', 'jetpack' ),
title: __( 'About Jetpack', 'jetpack' ),
href: getRedirectUrl( 'jetpack-about' ),
target: '_blank',
onClick: onAboutClick,
},
{
label: _x( 'Privacy', 'Shorthand for Privacy Policy.', 'jetpack' ),
title: __( "Automattic's Privacy Policy", 'jetpack' ),
href: areAdminLinksEnabled
? new URL( 'admin.php?page=jetpack#/privacy', siteAdminUrl ).href
: getRedirectUrl( 'a8c-privacy' ),
target: areAdminLinksEnabled ? '_self' : '_blank',
onClick: onPrivacyClick,
},
{
label: _x( 'Terms', 'Shorthand for Terms of Service.', 'jetpack' ),
title: __( 'WordPress.com Terms of Service', 'jetpack' ),
href: getRedirectUrl( 'wpcom-tos' ),
target: '_blank',
onClick: onTermsClick,
},
];

if ( menu ) {
items = [ ...items, ...menu ];
}

const jetpackItemContent = (
<>
<JetpackIcon />
Expand Down Expand Up @@ -59,7 +117,7 @@ const JetpackFooter: React.FC< JetpackFooterProps > = ( {
jetpackItemContent
) }
</li>
{ menu?.map( item => {
{ items.map( item => {
const isButton = item.role === 'button';
const isExternalLink = ! isButton && item.target === '_blank';

Expand All @@ -85,7 +143,14 @@ const JetpackFooter: React.FC< JetpackFooterProps > = ( {
);
} ) }
<li className="jp-dashboard-footer__a8c-item">
<a href={ a8cLogoHref } aria-label={ __( 'An Automattic Airline', 'jetpack' ) }>
<a
href={
areAdminLinksEnabled
? new URL( 'admin.php?page=jetpack_about', siteAdminUrl ).href
: getRedirectUrl( 'a8c-about' )
}
aria-label={ __( 'An Automattic Airline', 'jetpack' ) }
>
<AutomatticBylineLogo aria-hidden="true" />
</a>
</li>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ export default {
const Template: ComponentStory< typeof JetpackFooter > = args => <JetpackFooter { ...args } />;

const DefaultArgs = {
a8cLogoHref: 'https://automattic.com',
moduleName: 'Jetpack',
className: '',
moduleNameHref: 'https://jetpack.com',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,75 @@ exports[`JetpackFooter Render the component should match the snapshot: all props
Test module
</a>
</li>
<li>
<a
class="jp-dashboard-footer__menu-item is-external"
href="https://jetpack.com/redirect/?source=jetpack-about"
rel="noopener noreferrer"
target="_blank"
title="About Jetpack"
>
About
<svg
aria-hidden="true"
focusable="false"
height="16"
viewBox="0 0 24 24"
width="16"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M19.5 4.5h-7V6h4.44l-5.97 5.97 1.06 1.06L18 7.06v4.44h1.5v-7Zm-13 1a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2v-3H17v3a.5.5 0 0 1-.5.5h-10a.5.5 0 0 1-.5-.5v-10a.5.5 0 0 1 .5-.5h3V5.5h-3Z"
/>
</svg>
</a>
</li>
<li>
<a
class="jp-dashboard-footer__menu-item is-external"
href="https://jetpack.com/redirect/?source=a8c-privacy"
rel="noopener noreferrer"
target="_blank"
title="Automattic's Privacy Policy"
>
Privacy
<svg
aria-hidden="true"
focusable="false"
height="16"
viewBox="0 0 24 24"
width="16"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M19.5 4.5h-7V6h4.44l-5.97 5.97 1.06 1.06L18 7.06v4.44h1.5v-7Zm-13 1a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2v-3H17v3a.5.5 0 0 1-.5.5h-10a.5.5 0 0 1-.5-.5v-10a.5.5 0 0 1 .5-.5h3V5.5h-3Z"
/>
</svg>
</a>
</li>
<li>
<a
class="jp-dashboard-footer__menu-item is-external"
href="https://jetpack.com/redirect/?source=wpcom-tos"
rel="noopener noreferrer"
target="_blank"
title="WordPress.com Terms of Service"
>
Terms
<svg
aria-hidden="true"
focusable="false"
height="16"
viewBox="0 0 24 24"
width="16"
xmlns="http://www.w3.org/2000/svg"
>
<path
d="M19.5 4.5h-7V6h4.44l-5.97 5.97 1.06 1.06L18 7.06v4.44h1.5v-7Zm-13 1a2 2 0 0 0-2 2v10a2 2 0 0 0 2 2h10a2 2 0 0 0 2-2v-3H17v3a.5.5 0 0 1-.5.5h-10a.5.5 0 0 1-.5-.5v-10a.5.5 0 0 1 .5-.5h3V5.5h-3Z"
/>
</svg>
</a>
</li>
<li>
<a
class="jp-dashboard-footer__menu-item"
Expand Down Expand Up @@ -81,7 +150,7 @@ exports[`JetpackFooter Render the component should match the snapshot: all props
>
<a
aria-label="An Automattic Airline"
href="https://automattic.com"
href="https://jetpack.com/redirect/?source=a8c-about"
>
<svg
aria-hidden="true"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ describe( 'JetpackFooter', () => {
const className = 'sample-classname';
const moduleName = 'Test module';
const moduleNameHref = 'https://jetpack.com/path/to-some-page';
const a8cLogoHref = 'https://automattic.com';

describe( 'Render the component', () => {
const menu = [
Expand Down Expand Up @@ -71,12 +70,11 @@ describe( 'JetpackFooter', () => {
} );

it( 'should render the Automattic logo', () => {
render( <JetpackFooter a8cLogoHref={ a8cLogoHref } /> );
render( <JetpackFooter /> );

const element = screen.getByLabelText( 'An Automattic Airline', { selector: 'a' } );

expect( element ).toBeInTheDocument();
expect( element ).toHaveAttribute( 'href', a8cLogoHref );
} );

it( 'should render a list', () => {
Expand All @@ -86,7 +84,7 @@ describe( 'JetpackFooter', () => {

expect( element ).toBeInTheDocument();
// eslint-disable-next-line testing-library/no-node-access
expect( element.children ).toHaveLength( 2 + menu.length );
expect( element.children ).toHaveLength( 2 + 3 + menu.length ); // 2 logos, 3 generic links
} );

it( 'should render the links', () => {
Expand All @@ -113,7 +111,6 @@ describe( 'JetpackFooter', () => {
className={ className }
moduleName={ moduleName }
moduleNameHref={ moduleNameHref }
a8cLogoHref={ a8cLogoHref }
menu={ menu }
/>
);
Expand Down
27 changes: 21 additions & 6 deletions projects/js-packages/components/components/jetpack-footer/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
type JetpackFooterMenuItem = {
export type JetpackFooterMenuItem = {
href: string;
label: string;
onClick?: () => void;
Expand All @@ -9,11 +9,6 @@ type JetpackFooterMenuItem = {
};

export type JetpackFooterProps = {
/**
* Link for 'An Automattic Airline'.
*/
a8cLogoHref?: string;

/**
* Name of the module, e.g. 'Jetpack Search'.
*/
Expand All @@ -33,4 +28,24 @@ export type JetpackFooterProps = {
* Navigation menu to display in the footer.
*/
menu?: JetpackFooterMenuItem[];

/**
* URL of the site WP Admin.
*/
siteAdminUrl?: string;

/**
* Function called when the About link is clicked.
*/
onAboutClick?: () => void;

/**
* Function called when the Privacy link is clicked.
*/
onPrivacyClick?: () => void;

/**
* Function called when the Terms link is clicked.
*/
onTermsClick?: () => void;
};
3 changes: 2 additions & 1 deletion projects/js-packages/components/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@automattic/jetpack-components",
"version": "0.40.1-alpha",
"version": "0.41.0-alpha",
"description": "Jetpack Components Package",
"author": "Automattic",
"license": "GPL-2.0-or-later",
Expand All @@ -19,6 +19,7 @@
"@wordpress/browserslist-config": "5.19.0",
"@wordpress/components": "25.2.0",
"@wordpress/compose": "6.13.0",
"@wordpress/data": "9.6.0",
"@wordpress/date": "4.36.0",
"@wordpress/element": "5.13.0",
"@wordpress/i18n": "4.36.0",
Expand Down
4 changes: 0 additions & 4 deletions projects/packages/my-jetpack/_inc/style.module.scss
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@
--wp-admin-theme-color-darker-20: var(--jp-black-80);
height: 100%;

.jetpack-logo {
height: 40px;
}

.jp-dashboard-footer__jetpack-symbol {
height: 16px;
}
Expand Down
Loading