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.

Sign up for GitHub

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

Jump to bottom

SEO Tools: add archive_title page title token #20920

Merged
merged 9 commits into from
Sep 10, 2021
Merged

SEO Tools: add archive_title page title token #20920

merged 9 commits into from
Sep 10, 2021

Conversation

samiff
Copy link
Contributor

@samiff samiff commented Sep 1, 2021 •
edited

Changes proposed in this Pull Request:

  • For the custom Archives page title structure in SEO Tools, add a more generic archive_title token which builds on the current date token.
  • Usage of [date] will continue to work on a site, but we'll hide the "Date" insertion button in favor of "Archive Title".
  • This will allow sites to display proper names on custom post type archives (e.g. "Projects" for the JP CPT at /portfolio), while still showing the date on date-based archives.
  • Fixes SEO Tools: Archive titles only support the date archive pages, not any other archives (CPT, ...) #8918

Jetpack product discussion

#8918
Automattic/wp-calypso#56084

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

  • With changes pulled down, on a Jetpack connected site, head to /wp-admin/admin.php?page=jetpack#/traffic
  • Make sure Customize your SEO settings is enabled.
  • Click on Expand to customize the page title structures of your site.
  • For Archives use the buttons to insert each token presented (Site Title, Tagline, and Archive Title).
  • Since we are no longer showing the + Date button, manually type in [date].
  • You should now see an updated preview beneath the input. Both [date] and [archive_title] will show "Example Archive Title/Date" in preview.
  • Click Save settings and refresh to make sure the custom Archive title persists.
  • Check a custom post type archive page. For example you can enable Jetpack's custom Portfolio type, then view that archive at example.com/portfolio/. In the <title> you should see "Projects" replacing the [archive_title] and [date] tokens.
  • Check a date archive (e.g. example.com/2019/09. Here you should see the appropriate date replacing the [archive_title] and [date] tokens.

Next, spin up the Calypso portion: Automattic/wp-calypso#56084

  • Navigate to calypso.localhost:3000/marketing/traffic/example.com
  • Find the Archives section and make sure you see the correct custom Archives title you set in WP Admin.
  • Try removing the existing Date and Archive Title token by clicking, and then adding them again.
  • Customize your title a bit more and save. Then refresh and make sure it persists.
  • Again check on the frontend portfolio and date-based archive pages to make sure the correct <title> comes through.
  • To note, the Date button will only be replaced with the Archive Title button on Jetpack versions >= 10.2-alpha

Before:

1

After:

2

Calypso After:

3

Example /portfolio title:

Custom Archives Title - Site Title: Jurassic Testing - Tagline: Just another WordPress site - Date: Projects - Archive Title: Projects

Example /2021/09/ title:

Custom Archives Title - Site Title: Jurassic Testing - Tagline: Just another WordPress site - Date: September 2021 - Archive Title: September 2021

@samiff samiff added [Status] In Progress [Feature] SEO Tools Tools for improving a site's search engine optimization. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ labels Sep 1, 2021
@samiff samiff added this to the jetpack/10.2 milestone Sep 1, 2021
@samiff samiff self-assigned this Sep 1, 2021
@matticbot
Copy link
Contributor

Caution: This PR has changes that must be merged to WordPress.com
Hello samiff! These changes need to be synced to WordPress.com - If you 're an a11n, please commandeer and confirm D66237-code works as expected before merging this PR. Once this PR is merged, please commit the changes to WP.com. Thank you!
This revision will be updated with each commit to this PR

@github-actions github-actions bot added the Admin Page React-powered dashboard under the Jetpack menu label Sep 1, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Sep 1, 2021 •
edited

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • ✅ Include a description of your PR changes.
  • ✅ All commits were linted before commit.
  • ✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • ✅ Add testing instructions.
  • ✅ Specify whether this PR includes any changes to data or privacy.
  • ✅ Add changelog entries to affected projects

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.
Then, add the "[Status] Needs Team review" label and ask someone from your team review the code.
Once you’ve done so, switch to the "[Status] Needs Review" label; someone from Jetpack Crew will then review this PR and merge it to be included in the next Jetpack release.

Jetpack plugin:

  • Next scheduled release: October 5, 2021.
  • Scheduled code freeze: September 28, 2021.

@samiff
Copy link
Contributor Author

samiff commented Sep 1, 2021

@jeherve Before I start poking around in Calypso, what do you think of this approach?

jeherve
jeherve reviewed Sep 3, 2021
View reviewed changes
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.

@jeherve Before I start poking around in Calypso, what do you think of this approach?

I think that makes perfect sense, it's more logical than before.

I would, however, recommend that we keep transforming the [date] token in the interface, to avoid things like this on sites that are already set up:

image

@samiff samiff mentioned this pull request Sep 7, 2021
Merged
@samiff
Copy link
Contributor Author

samiff commented Sep 7, 2021

recommend that we keep transforming the [date] token in the interface

That makes sense, I've changed things up so we are essentially only hiding the Date button now in wp-admin and Calypso. We still want the [date] tokenization to occur as normal, otherwise someone might not notice the change to [date] when saving other titles.

@samiff samiff added [Status] Needs Review To request a review from Crew. Label will be renamed soon. and removed [Status] In Progress labels Sep 7, 2021
jeherve
jeherve previously approved these changes Sep 8, 2021
View reviewed changes
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.

This tests well for me. I only have a minor remark, but that's not a blocker.

projects/plugins/jetpack/_inc/client/traffic/seo/custom-seo-titles.jsx Outdated Show resolved Hide resolved
@jeherve jeherve added [Status] Ready to Merge Go ahead, you can push that green button! and removed [Status] Needs Review To request a review from Crew. Label will be renamed soon. labels Sep 8, 2021
@samiff @jeherve
Update projects/plugins/jetpack/_inc/client/traffic/seo/custom-seo-ti…
a19070f 
…tles.jsx


Capture group not needed

Co-authored-by: Jeremy Herve <jeremy@jeremy.hu>
@samiff samiff removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 8, 2021
@samiff samiff added the DO NOT MERGE don't merge it! label Sep 8, 2021
@github-actions github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Sep 8, 2021
@samiff
Copy link
Contributor Author

samiff commented Sep 8, 2021

@jeherve Could you help me wrap my head around the wp-admin vs Calypso side of this.

If the Calypso side is deployed, then connected WPorg JP sites will see the button to insert the Archive Title in Calypso, but this will only be saveable if they are running JP >= 10.2 which adds the archive_title to allowed tokens in jetpack-seo-titles.php. Otherwise they will get an error when saving custom titles with an Archive Title in it.

Is a JP version check needed when adding the Archive Title button on the Calypso side, or is there a better way to reconcile this?

@jeherve
Copy link
Member

jeherve commented Sep 9, 2021

Is a JP version check needed when adding the Archive Title button on the Calypso side, or is there a better way to reconcile this?

That's what I would recommend, yes. Calypso includes utility functions to help you with this.

@samiff samiff added [Status] In Progress 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 9, 2021
@samiff samiff added [Status] Ready to Merge Go ahead, you can push that green button! and removed DO NOT MERGE don't merge it! [Status] In Progress labels Sep 10, 2021
@samiff samiff merged commit 5e22f92 into master Sep 10, 2021
@samiff samiff deleted the add/seo-archive-title branch September 10, 2021 18:26
@github-actions
Copy link
Contributor

Great news! One last step: head over to your WordPress.com diff, D66237-code, and commit it.
Once you've done so, come back to this PR and add a comment with your changeset ID.

Thank you!

@samiff
Copy link
Contributor Author

samiff commented Sep 10, 2021

r231579-wpcom

@samiff samiff removed the [Status] Ready to Merge Go ahead, you can push that green button! label Sep 10, 2021
davidlonjon added a commit that referenced this pull request Sep 14, 2021
@davidlonjon
Merge branch 'master' into fix/boost-linting-setup-and-standards
03e9a94 
* master: (32 commits)
  jetpack: Work around another calypso-build oddity (#21004)
  Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922)
  Search: Improve filter label alignment for longer terms (#21008)
  Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993)
  Notifications: fix DailyMotion shortcode render in Calypso (#21032)
  Notifications: Fix VR shortcode render (#20871)
  E2E tests: fix for logging Jetpack version (#21054)
  Admin Menu: Fix skipped tests (#21055)
  Admin Menu: Sanitize current screen identifier (#21049)
  Fix/phpunit failed on multisite (#21053)
  Boost: Fix Critical CSS resume on module re-enable (#21031)
  Update wordpress monorepo (#21021)
  Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045)
  Publicize: move it to plugins folder (#21016)
  SEO Tools: add archive_title page title token (#20920)
  Unit tests: Fix silent errors in Jetpack tests (#21030)
  actions: Don't run Beta Plugin artifact action on forked PRs (#21019)
  Jetpack Sync: Fix broken Sync unit tests (#21037)
  Notifications: Fix SoundCloud shortcode render (#20869)
  Jetpack Assistant: Correct purchased product feature class (#21014)
  ...
davidlonjon added a commit that referenced this pull request Sep 14, 2021
@davidlonjon
[not verified] Merge branch 'master' into fix/boost-linting-typescript
e430d79 
# By Ivan Ottinger (4) and others
# Via GitHub
* master: (28 commits)
  Boost: Fix linting config and standards in files (#21015)
  jetpack: Work around another calypso-build oddity (#21004)
  Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922)
  Search: Improve filter label alignment for longer terms (#21008)
  Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993)
  Notifications: fix DailyMotion shortcode render in Calypso (#21032)
  Notifications: Fix VR shortcode render (#20871)
  E2E tests: fix for logging Jetpack version (#21054)
  Admin Menu: Fix skipped tests (#21055)
  Admin Menu: Sanitize current screen identifier (#21049)
  Fix/phpunit failed on multisite (#21053)
  Boost: Fix Critical CSS resume on module re-enable (#21031)
  Update wordpress monorepo (#21021)
  Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045)
  Publicize: move it to plugins folder (#21016)
  SEO Tools: add archive_title page title token (#20920)
  Unit tests: Fix silent errors in Jetpack tests (#21030)
  actions: Don't run Beta Plugin artifact action on forked PRs (#21019)
  Jetpack Sync: Fix broken Sync unit tests (#21037)
  Notifications: Fix SoundCloud shortcode render (#20869)
  ...

# Conflicts:
#	projects/plugins/boost/tsconfig.json
davidlonjon added a commit that referenced this pull request Sep 14, 2021
@davidlonjon
[not verified] Merge branch 'master' into try/boost-javascript-tests-…
76b7b4a 
…workflow

# By Brad Jorsch (6) and others
# Via GitHub
* master: (53 commits)
  Boost: Fix linting config and standards in files (#21015)
  jetpack: Work around another calypso-build oddity (#21004)
  Fix/20806 Browser console deprecation warning for BlcokControl entity (#20922)
  Search: Improve filter label alignment for longer terms (#21008)
  Theme Tools: rm extra Social Menu added in Twenty Twenty theme (#20993)
  Notifications: fix DailyMotion shortcode render in Calypso (#21032)
  Notifications: Fix VR shortcode render (#20871)
  E2E tests: fix for logging Jetpack version (#21054)
  Admin Menu: Fix skipped tests (#21055)
  Admin Menu: Sanitize current screen identifier (#21049)
  Fix/phpunit failed on multisite (#21053)
  Boost: Fix Critical CSS resume on module re-enable (#21031)
  Update wordpress monorepo (#21021)
  Unit tests: test_get_user_connection_data_with_connected_user skip for multisite (#21045)
  Publicize: move it to plugins folder (#21016)
  SEO Tools: add archive_title page title token (#20920)
  Unit tests: Fix silent errors in Jetpack tests (#21030)
  actions: Don't run Beta Plugin artifact action on forked PRs (#21019)
  Jetpack Sync: Fix broken Sync unit tests (#21037)
  Notifications: Fix SoundCloud shortcode render (#20869)
  ...

# Conflicts:
#	projects/plugins/boost/tsconfig.json
#	projects/plugins/jetpack/tests/e2e/bin/env.sh
@samiff samiff added the [Status] Needs Testing We need to add this change to the testing call for this month's release label Sep 28, 2021
@jeherve jeherve removed the [Status] Needs Testing We need to add this change to the testing call for this month's release label Oct 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdixon194 sdixon194 sdixon194 approved these changes

@jeherve jeherve jeherve left review comments

Assignees

@samiff samiff

Labels
Admin Page React-powered dashboard under the Jetpack menu [Feature] SEO Tools Tools for improving a site's search engine optimization. [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ Touches WP.com Files
Projects
None yet
Milestone
jetpack/10.2
Development

Successfully merging this pull request may close these issues.

SEO Tools: Archive titles only support the date archive pages, not any other archives (CPT, ...)
4 participants
@samiff @matticbot @jeherve @sdixon194