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

adding slash to gen.go links for seo reasons #11569

Merged
merged 4 commits into from Dec 8, 2022
Merged

adding slash to gen.go links for seo reasons #11569

merged 4 commits into from Dec 8, 2022

Conversation

susanev
Copy link
Contributor

@susanev susanev commented Dec 6, 2022

Description

seo requires that links end consistently.
we care about seo because it impacts organic search.

because we use hugo, we have a constraint that links need to end in a slash.
but we have many links that don't end in a slash.
this is attempting to fix some of those links.
important note: anchor links do not matter, and do not need the slash at the end.

there's another layer to this work which is fixing up the links that come from GetDocLinkForPulumiType but ill take that on in a separate pr.

you can view a preview of the docs here:
http://pulumi-docs-origin-pr-8310-e2322762.s3-website.us-west-2.amazonaws.com/registry

the majority of what this is fixing is adding a slash to the end of links on the top-level API docs pages, and the module pages. you can hover over any of those links in a package to confirm they have the slash.

Screen Shot 2022-12-07 at 3 03 11 PM

Fixes # (issue)

Checklist

  • I have added tests that prove my fix is effective or that my feature works
  • I have run make changelog and committed the changelog/pending/<file> documenting my change
  • Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

Signed-off-by: susanev <susan.ra.evans@gmail.com>
@susanev susanev self-assigned this Dec 6, 2022
@pulumi-bot
Copy link
Contributor

pulumi-bot commented Dec 6, 2022

Changelog

[uncommitted] (2022-12-07)

Signed-off-by: susanev <susan.ra.evans@gmail.com>
@susanev susanev added the impact/no-changelog-required This issue doesn't require a CHANGELOG update label Dec 6, 2022
@susanev susanev changed the title adding slash to gen dot go adding slash to gen.go links for seo reasons Dec 6, 2022
@susanev
Copy link
Contributor Author

susanev commented Dec 6, 2022

hello!

@cnunciato @kimberleyamackenzie @justinvp do you mind taking a quick look at this and letting me know if im doing things horribly wrong before i include some more changes? thank you!

Signed-off-by: susanev <susan.ra.evans@gmail.com>
Signed-off-by: susanev <susan.ra.evans@gmail.com>
@susanev susanev marked this pull request as ready for review December 7, 2022 23:03
@susanev susanev requested review from kimberleyamackenzie and a team December 7, 2022 23:03
Copy link
Contributor

@kimberleyamackenzie kimberleyamackenzie left a comment

Choose a reason for hiding this comment

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

I poked around the build, and everything looks good to me! I picked a few random packages, and verified links on the API docs index pages (modules, resources, and functions) all had the trailing slash, and the same on the index pages for individual modules. Also verified the Kubernetes case with modules inside modules. 🎉 Thanks for doing this!!

@@ -1846,7 +1846,7 @@ func (mod *modContext) genIndex() indexData {
for _, r := range mod.resources {
name := resourceName(r)
resources = append(resources, indexEntry{
Link: getResourceLink(name),
Link: getResourceLink(name) + "/",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm tempted to say this should be some kind of conditional that checks whether there's already a trailing slash (defense in depth and all that), and that should probably occur within the getResourceLink function.

Just my 2c. 🪙 It's late here, otherwise I'd offer a more concrete suggestion, apologies! I'll revisit tomorrow during work hours when I triage my notifications.

Copy link
Contributor Author

@susanev susanev Dec 8, 2022

Choose a reason for hiding this comment

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

would you be okay with that change happening after this pr lands? I'm happy to revisit this in more detail later on if you'd be up for pairing.

right now none of the resources and function links end in slashes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think there's prolly some other issues in this file as well if we want to go with your suggested approach, specifically with anchors getting the octothorpe added in this file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me!

Copy link
Contributor

@RobbieMcKinstry RobbieMcKinstry left a comment

Choose a reason for hiding this comment

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

LGTM. If we want to tidy, we can follow on with another PR later.

@susanev
Copy link
Contributor Author

susanev commented Dec 8, 2022

bors merge

@bors
Copy link
Contributor

bors bot commented Dec 8, 2022

Build succeeded:

@bors bors bot merged commit 711a3e9 into master Dec 8, 2022
@bors bors bot deleted the se/docs-gen-links branch December 8, 2022 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
impact/no-changelog-required This issue doesn't require a CHANGELOG update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants