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

Tidy <head> for pages #48350

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Tidy <head> for pages #48350

merged 1 commit into from
Jan 27, 2025
+81 −74

Conversation

sftim
Copy link
Contributor

@sftim sftim commented Oct 15, 2024 •
edited
Loading

  • Remove duplicated Twitter and Open Graph metadata
  • Render the Open Graph image through Hugo Pipes
  • Move the theme color to the head-end theme hook

Here's a preview

Helps with issue #41171

/area web-development

@k8s-ci-robot k8s-ci-robot added the area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes label Oct 15, 2024
@k8s-ci-robot k8s-ci-robot added the sig/docs Categorizes an issue or PR as relevant to SIG Docs. label Oct 15, 2024
@k8s-ci-robot k8s-ci-robot requested a review from tengqm October 15, 2024 11:51
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2024
sftim
sftim commented Oct 15, 2024
View reviewed changes
layouts/partials/head.html
@@ -52,27 +49,44 @@
{{ end }}
}
</script>
<meta name="theme-color" content="#326ce5">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to layouts/partials/hooks/head-end.html

sftim
sftim commented Oct 15, 2024
View reviewed changes
layouts/partials/head.html Outdated
Comment on lines 25 to 27
{{- template "_internal/opengraph.html" . -}}
{{- template "_internal/schema.html" . -}}
{{- template "_internal/twitter_cards.html" . -}}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing these internal template calls as we are customizing them later in the same layout file.

@netlify Netlify
Copy link

netlify bot commented Oct 15, 2024 •
edited
Loading

✅ Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit ee50a91
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/679377351b07a900080b36b8
😎 Deploy Preview https://deploy-preview-48350--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@sftim sftim force-pushed the 20241015_tidy_page_head branch from e776fce to 6ad472b Compare October 26, 2024 20:31
@sftim sftim force-pushed the 20241015_tidy_page_head branch from 6ad472b to 3b062b7 Compare November 18, 2024 14:00
@sftim sftim force-pushed the 20241015_tidy_page_head branch from 3b062b7 to 1b00c5a Compare December 2, 2024 12:52
@sftim sftim force-pushed the 20241015_tidy_page_head branch from 1b00c5a to 328cc60 Compare December 12, 2024 10:06
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 31, 2024
@sftim sftim force-pushed the 20241015_tidy_page_head branch from 328cc60 to 82e20bd Compare January 14, 2025 22:32
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 14, 2025
@sftim sftim mentioned this pull request Jan 14, 2025
Open
sftim
sftim commented Jan 21, 2025
View reviewed changes
layouts/partials/head.html
{{ partial "hooks/head-end.html" . }}
{{/* To comply with GDPR, cookie consent scripts places in head-end must execute before Google Analytics is enabled */ -}}
{{ if hugo.IsProduction -}}
{{ template "_internal/google_analytics.html" . }}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what's changed from upstream Docsy

@sftim sftim force-pushed the 20241015_tidy_page_head branch from 82e20bd to 7d99045 Compare January 24, 2025 10:57
@sftim
Tidy <head> for pages
ee50a91 
- Align closely to Docsy theme
- Remove duplicated Twitter and Open Graph metadata
- Render the Open Graph image through Hugo Pipes
- Move the theme color to the head-end hook
@sftim sftim force-pushed the 20241015_tidy_page_head branch from 7d99045 to ee50a91 Compare January 24, 2025 11:19
@sftim
Copy link
Contributor Author

sftim commented Jan 24, 2025

I did notice that the Open Graph image link was missing, so I added that back in.

@natalisucks
Copy link
Contributor

thanks @sftim 👍
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 27, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 5d6ae2d9bd88e9a7466e25ed40eefc8022121373

@sftim
Copy link
Contributor Author

sftim commented Jan 27, 2025

@salaxander @katcosgrove @tengqm happy to see this merge?

@katcosgrove
Copy link
Contributor

Yeah, no issue here. Thanks!

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: katcosgrove

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 27, 2025
@k8s-ci-robot k8s-ci-robot merged commit af0077f into kubernetes:main Jan 27, 2025
6 checks passed
@Shubham82
Copy link
Contributor

@sftim Hi,
I think the changes you made resulted in the following output:
Screenshot1
Screenshot2

Could you please check it?

Deploy preview of your PR: https://deploy-preview-48350--kubernetes-io-main-staging.netlify.app/docs/home/

@sftim sftim deleted the 20241015_tidy_page_head branch January 27, 2025 14:04
@sftim
Copy link
Contributor Author

sftim commented Jan 27, 2025

Hi @Shubham82

Is there a page you'd like me to look at?
You are very welcome to file an issue (no permission needed).

@natalisucks
Copy link
Contributor

@Shubham82 thanks so much for commenting again, it certainly does look like an issue on the live site now, but that wasn't the case for the first preview of Tim's changes.

@sftim its looks like now every page seems to load the left navigation bar and leave the rest of the space to the right of navigation blank, causing users to scroll down to see content. i didn't see this in the first preview of your changes, but perhaps the second set of changes you pushed has caused this?

please check https://kubernetes.io/docs/setup/, or https://kubernetes.io/docs/tutorials/ as examples

@sftim sftim mentioned this pull request Jan 27, 2025
Closed
@Shubham82
Copy link
Contributor

@sftim, not a specific page(attached it for reference), when we click on documentation in k8s docs (website), it shows like in the above screenshot.
https://kubernetes.io/docs/home/

@sftim sftim mentioned this pull request Jan 27, 2025
Closed
@Shubham82
Copy link
Contributor

@Shubham82 thanks so much for commenting again, it certainly does look like an issue on the live site now, but that wasn't the case for the first preview of Tim's changes.

@natalisucks, Anytime

@sftim sftim mentioned this pull request Jan 27, 2025
Merged
@shurup shurup mentioned this pull request Jan 29, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@natalisucks natalisucks natalisucks left review comments

@salaxander salaxander Awaiting requested review from salaxander

@tengqm tengqm Awaiting requested review from tengqm

Assignees

@natalisucks natalisucks

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/docs Categorizes an issue or PR as relevant to SIG Docs. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sftim @natalisucks @k8s-ci-robot @katcosgrove @Shubham82