-
Notifications
You must be signed in to change notification settings - Fork 15.1k
[WIP] Align to Docsy theme #48363
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
[WIP] Align to Docsy theme #48363
Conversation
✅ Pull request preview available for checkingBuilt without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
e690c92 to
ba2190e
Compare
ba2190e to
a6de947
Compare
c6f85e1 to
65a6340
Compare
65a6340 to
1116c85
Compare
1116c85 to
ec2586b
Compare
ec2586b to
c6f85e1
Compare
c6f85e1 to
ec2586b
Compare
b959c4c to
792471e
Compare
|
Sharing a comment on #41171 about the menu display issue. 🙂 |
a63a2a2 to
c05c31e
Compare
c05c31e to
38c3ffa
Compare
38c3ffa to
0cf140d
Compare
5bf5894 to
0c45173
Compare
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.
Pull Request Overview
This PR updates the site's theme to more closely align with the vanilla Docsy theme while adding and adjusting search functionality and minor UI behaviors. Key changes include:
- Introducing a new search script (search-google.js) for handling Google search.
- Enhancing search.js with both Google and Pagefind search fallbacks plus additional DOM manipulations.
- Modifying hugo.toml to update search-related configuration.
- Adjusting banner dismissal behavior in banner-dismiss.js via a jQuery refactor.
Reviewed Changes
Copilot reviewed 74 out of 74 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| assets/js/search-google.js | New search script added to support Google search integration. |
| assets/js/search.js | Refactored search behavior with added handling for fallback search methods. |
| hugo.toml | Updated configuration for search UI and theme parameters. |
| assets/js/banner-dismiss.js | Switched to jQuery methods for announcement dismissal with minor logic change. |
assets/js/search.js
Outdated
| sidebarSearch.remove(); | ||
| } | ||
| document.getElementById('search').style.display = 'block'; | ||
| pagefind = new PagefindUI({ element: "#search", showImages: false }); |
Copilot
AI
Mar 14, 2025
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.
The 'pagefind' variable is assigned without declaration, which can lead to unintended global variable creation. Consider declaring it with 'let' or 'const'.
| pagefind = new PagefindUI({ element: "#search", showImages: false }); | |
| let pagefind = new PagefindUI({ element: "#search", showImages: false }); |
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.
@SayakMukhopadhyay FYI!
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.
Nice find, I have updated #49724 with let or const as needed.
| } | ||
| else { | ||
| announcement.classList.add('display-announcement') // Display the announcement if the cookie is not set | ||
| announcement.removeClass('announcement-shown').addClass('announcement-shown'); // Display the announcement if the cookie is not set |
Copilot
AI
Mar 14, 2025
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.
Removing and then immediately re-adding the same 'announcement-shown' class seems redundant; review the intended behavior for displaying the announcement.
| announcement.removeClass('announcement-shown').addClass('announcement-shown'); // Display the announcement if the cookie is not set | |
| announcement.addClass('announcement-shown'); // Display the announcement if the cookie is not set |
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 think the idea is to make sure the class is only asserted once. Not sure.
0c45173 to
69d9bd0
Compare
69d9bd0 to
b0d845d
Compare
b0d845d to
bf8fc73
Compare
Redo the custom theme to be closer to vanilla Docsy.
bf8fc73 to
3e51135
Compare
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Superseded by PR #50665 /close |
Redo the custom theme to be closer to vanilla Docsy.
Here's a preview
Helps with issue #41171
/area web-development
This PR is too large to merge as-is.
/hold
until it is small enough to be reviewable as a single change (eg size/XL or smaller)
I will carve off smaller PRs that are reviewable.