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

Infinite Scroll: Add privacy policy link #9918

Merged

Conversation

laghee
Copy link
Contributor

@laghee laghee commented Jul 15, 2018

Fixes #9435

WP bundled themes now have a privacy policy link in their footer, which
is helpful for complying with GDPR rules, but users who enable the infinite
footer lose that link. This adds a similar link to the infinity footer $credits.

Changes proposed in this Pull Request:

  • Add a privacy policy link to $credits to mimic the new WP bundled theme link

Testing instructions:

  • Publish the privacy policy page on test site
  • Enable infinite scroll with this patch applied
  • Scroll down to display the infinite footer
  • Confirm that it now includes a link to the privacy policy page

(Edit: Added extra step for testing.)

@laghee laghee requested a review from a team as a code owner July 15, 2018 17:51
@abidhahmed abidhahmed added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jul 17, 2018
@abidhahmed abidhahmed added this to the 6.4 milestone Jul 17, 2018
@brbrr
Copy link
Contributor

brbrr commented Jul 24, 2018

Hi @laghee. Thanks for your contribution!

I've tested your PR, but I wasn't able to see Privacy Policy link in IS footer:
(IS Enabled)
screen shot on 2018-07-24 at 17 29 49

(IS Disabled)
helloword just another wordpress site 2018-07-24 17-32-31

Also, as you can see - there is redundant '/' just before "Proudly powered by WordPress" text. So I would expect that the_privacy_policy_link() return an empty string in my case.

@brbrr brbrr added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! [Status] In Progress and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Testing We need to add this change to the testing call for this month's release labels Jul 24, 2018
@brbrr brbrr modified the milestones: 6.4, 6.5 Jul 31, 2018
@brbrr
Copy link
Contributor

brbrr commented Jul 31, 2018

Moving it into next release

cc @laghee

@laghee
Copy link
Contributor Author

laghee commented Jul 31, 2018

Thanks, @brbrr ! You're right -- I can see this locally (now that I unbroke my MAMP setup).

I did some investigation, and it's not that the_privacy_policy_link() is returning an empty string, but rather, it's not inserting the link into the credits class, so the link is just floating around in the ether, in its privacy policy link class, above the infinite footer div ... 🤔 ... weird.

screenshot 2018-07-31 17 27 38

I'll poke around a little to see if I can figure out what's going wrong and try to push a fix in the next couple of days.

@laghee laghee force-pushed the add/privacy-policy-infinite-footer branch from 09958a5 to 765eb5f Compare August 1, 2018 13:34
@laghee
Copy link
Contributor Author

laghee commented Aug 1, 2018

@brbrr I changed the retrieval method to use get_privacy_policy_url() instead of the_privacy_policy_link, and that appears to have fixed the problem. This works for me locally both with the privacy policy page enabled and not.

Using the_privacy_policy_link meant the link was coming back already formatted, so it wasn't being inserted into the blog-credits div. (It also made the link text inaccessible for easy translation.)

The Travis checks look like they have to do with a new build configuration and not anything I did, but let me know if I need to tweak something.

@brbrr brbrr added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] In Progress [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Aug 3, 2018
@brbrr
Copy link
Contributor

brbrr commented Aug 3, 2018

Thanks for your contribution! Now I can see the Privacy Policy link.
screen shot on 2018-08-03 at 18 30 36

Copy link
Contributor

@brbrr brbrr left a comment

Choose a reason for hiding this comment

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

Functionality reviewed - LGTM 🚢

$credits = sprintf(
'<a href="%1$s" rel="noopener noreferrer" target="_blank">%2$s</a><span role="separator" aria-hidden="true"> / </span>',
get_privacy_policy_url(),
__('Privacy Policy', 'jetpack' )
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing space between '(' & 'Privacy Policy'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brbrr Oops! I can push another commit right now. I assume I should amend the last one so it's not just a typo fix?

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries about number of commits. We are squashing before merge. Thanks!

@brbrr brbrr added [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 7, 2018
@mdawaffe
Copy link
Member

Nice! I see that the default themes (twentyten, etc.) all use the_privacy_policy_link()/get_the_privacy_policy_link(), which uses the Privacy Policy's page title as the link text. Should we be using those functions instead so that we use that title as well?

@mdawaffe mdawaffe added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Ready to Merge Go ahead, you can push that green button! labels Aug 14, 2018
@jetpackbot
Copy link

Warnings
⚠️

"Proposed changelog entry" is missing for this PR. Please include any meaningful changes

This is automated check which relies on PULL_REQUEST_TEMPLATE.We encourage you to follow that template as it helps Jetpack maintainers do their job. If you think 'Testing instructions' or 'Proposed changelog entry' are not needed for your PR - please explain why you think so. Thanks for cooperation 🤖

Generated by 🚫 dangerJS

@laghee
Copy link
Contributor Author

laghee commented Aug 15, 2018

@mdawaffe You're right -- that worked like a charm! I should have realized that since the privacy page is just another blog page, the title could be customized like anything else. Displaying just "Privacy Policy" would be annoying to anyone who had bothered to write their own title.

Thanks for the help!

@mdawaffe
Copy link
Member

Do we still need that <span role="separator" aria-hidden="true"> from before? I don't know :)

oskosk
oskosk previously requested changes Aug 28, 2018
*/
private function default_footer() {
$credits = sprintf(
if ( get_privacy_policy_url() !== '' ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we check if get_privacy_policy_url() is defined (Only WP 4.9.6 and later define it ) ?

@oskosk oskosk added [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. labels Aug 28, 2018
@oskosk oskosk removed this from the 6.5 milestone Aug 30, 2018
laghee added 5 commits August 31, 2018 22:01
WP bundled themes now have a privacy policy link in their footer, which
is helpful for complying with GDPR rules, but users who enable the infinite
footer lose that link. This adds a similar link to the infinity footer $credits.
As `the_privacy_policy_link` displays a pre-formatted link, it was not being incorporated into
the `blog-credits` div. The 'Privacy Policy' link text was also not easily accessible for
translation. Using `get_privacy_policy_url()` allowed for the formatting to match the rest of
the credits on the footer, and this secured the correct behavior.
…y_link()

As `get_privacy_policy_url()` comes complete with the privacy policy page's
title rather than set text, this is a better solution for displaying the link.
… span tag

Since `get_privacy_policy_url` is not defined before WP 4.9.6, this adds a
check to make the default infinite footer backward compatible.
@laghee laghee force-pushed the add/privacy-policy-infinite-footer branch from bee286a to 6ed51c9 Compare August 31, 2018 20:02
@laghee
Copy link
Contributor Author

laghee commented Aug 31, 2018

I made changes requested by @oskosk & @mdawaffe. Sorry about the delay!

@mdawaffe
Copy link
Member

mdawaffe commented Sep 4, 2018

Thanks for your persistence! Looks good to me.

@mdawaffe mdawaffe added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Sep 4, 2018
@mdawaffe mdawaffe added this to the 6.6 milestone Sep 4, 2018
@jeherve jeherve dismissed oskosk’s stale review September 7, 2018 12:10

We now check if the function exists before to use it.

- Use spaces inside parenthesis.
- Use Yoda condition.
Copy link
Member

@jeherve jeherve left a comment

Choose a reason for hiding this comment

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

I made a small change for coding standards, but other than that I think we are all set. Merging.

Thank you for your contribution!

@jeherve jeherve merged commit 1b48b87 into Automattic:master Sep 7, 2018
@jeherve jeherve added [Status] Has Changelog and removed [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Status] Needs Changelog labels Sep 7, 2018
jeherve added a commit that referenced this pull request Sep 14, 2018
jeherve added a commit that referenced this pull request Sep 24, 2018
jeherve added a commit that referenced this pull request Sep 25, 2018
* Readme: add boilerplate for next release, 6.6

* Add 6.5 to the changelog.txt file

* Set boilerplate testing list for 6.6

* Readme: update stable tag to 6.5

* Add bullets to 6.5 changelog items

* Readme: add link to previous changelogs

This will help folks who want to know more about past releases,
while keeping the readme.txt short so as to not overwhelm translators and site owners only looking for information about the last release.

* Changelog: add information at the top of the changelog file.

* Changelog: add #10054

* Changelog: add #10078

* Changelog: add #10079

* Changelog: add #10064

* Changelog: add #10094

* Changelog: add #10096

* Testing list: add more information based on #10087

* Changelog: add #9847

* Changelog: add #10084

* Changelog: add #9918

* Changelog: add #7614

* Changelog: add #10116

* Changelog: add #10108

* Changelog: add #10041

* Changelog: add #10121

* Changelog: add #10134

* Changelog: add #10130

* Changelog: add #10109

* changelog: add #10137

* changelog: add #9952

* changelog: add #10120

* changelog: add #10162

* Changelog: add #10163

* Changelog: add #10092

* changelog: add #10156

* Changelog: add #10154

* changelog: add #10122

* Changelog: add #10101

* changelog: add #10105

* changelog: add #10190

* Changelog: add #10196

* changelog: add #10152

* Changelog: add #10153

* Testing list: add more details to Site Verification testing steps.

@see #10143 (comment)

* changelog: add #10194

* Changelog: add #10193
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Infinite Scroll [Type] Enhancement Changes to an existing feature — removing, adding, or changing parts of it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Infinite Scroll: Privacy Policy link missing from footer
8 participants