-
Notifications
You must be signed in to change notification settings - Fork 800
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
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Once your PR is ready for review, check one last time that all required checks (other than "Required review") appearing at the bottom of this PR are passing or skipped. Jetpack plugin:
Social plugin:
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
a2107bf
to
92bf77b
Compare
a8cLogoHref={ a8cLogoHref } | ||
moduleNameHref={ moduleNameHref } | ||
/> | ||
<JetpackFooter moduleName={ moduleName } moduleNameHref={ moduleNameHref } /> |
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.
href
of the Automattic logo is set in JetpackFooter
directly.
c6b92e6
to
53250cb
Compare
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'; |
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.
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.
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 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. :)
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.
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
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.
Another solution would be to use the data from the scrip-data package as explained in this thread.
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.
That said, composability is still better because that will not make the component dependent upon anything related to jetpack connection or its logic.
); | ||
|
||
const areAdminLinksEnabled = | ||
siteAdminUrl && |
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.
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.
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.
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' );
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.
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;
3a22723
to
9e01c1b
Compare
@jeherve the |
The logged-in About link on the Jetpack dashboard (and other) pages shows up as an external link, although it's not. However, it loads the About page in a new tab, which differs from the Terms and Airline pages. There are inconsistencies between the MyJetpack footer and other pages. This is what I see:
Privacy on connected MyJetpack is still an external link. I'm unsure if aligning these should be a part of another PR. |
Yeah, let's land this and open an issue to track the variations. |
@bindlegirl I think the idea of shipping as is, but, creating a follow up issue sounds as a good idea to me. |
Sounds good to me 👍 |
…etpack-footer-generic-links
Fixes #31598
Proposed changes:
The
JetpackFooter
component is used in the Jetpack flagship plugin and many other Jetpack plugins. In the former, it contains links to about, TOS, and privacy pages that are not included in the latter, though they can provide value in those instances as well. This PR integrates the About, Terms, and Privacy links into theJetpackFooter
component directly.It also removes from the props the URL to which the Automattic logo links to since all instances using
JetpackFooter
would pass the same URL.In more detail:
JetpackFooter
JetpackFooter
a8cLogoHref
fromJetpackFooter
An Automattic Airline
links to internal pagesOther information:
Jetpack product discussion
p1687876101340969-slack-C01264051NE
p1687874518864269-slack-C02TQF5VAJD
Does this pull request change what data or activity we track or use?
No.
Testing instructions:
https://jetpack.com/
https://jetpack.com/about/
https://automattic.com/privacy/
https://wordpress.com/tos/
https://automattic.com/about/
https://jetpack.com/
https://jetpack.com/about/
https://automattic.com/privacy/
https://wordpress.com/tos/
https://automattic.com/about/
https://jetpack.com/
https://jetpack.com/about/
/wp-admin/admin.php?page=jetpack#/privacy
https://wordpress.com/tos/
/wp-admin/admin.php?page=jetpack_about