Skip to content

Conversation

@SayakMukhopadhyay
Copy link
Member

@SayakMukhopadhyay SayakMukhopadhyay commented Feb 12, 2025

This is a small change towards getting the search functionality closer to Docsy. Its related to #47975 but takes a different approach to the same end. The changes made are as follows:

  1. The content from static/js/search.js and assets/js/search.js moved to assets/js/custom-search.js. This helps with modification and prevents the need to override search,js from docsy/assets/js. The file is also named custom-search.js in the same vein as Docsy has offline-search.js. This helps in indicating that their functionality are similar. Also updated all variable declarations to use let or const and all equality conditions to use ===.
  2. This new file custom-search.js is incorporated layouts/partials/scripts.html Docsy add all search scripts like the offline-search.js in this file. This addition is also done inside a .Site.Params.customSearch guard in the same vein that Docsy uses .Site.Params.offlineSearch guard.
    As an aside, I was thinking of seeing if a "custom search" feature could be added in Docsy which would mean that we would just need to add a script and be done with any custom search functionality.
  3. The linking of the deleted /static/js/search.js is removed from layouts/_default/search.html
  4. Moved the custom search UI implementation from search-input.html to search-input-custom.html. This utilises an unmerged upstream PR's basic idea. Also updates search-input.html itself by copying the rest of the code from Docsy to keep things as 1:1 aligned as possible.
  5. search.html has been updated to bring it as close to Docsy as possible. It replaces the majority of the body code including the search bar. The search bar now uses the same CSS styling as the search bar in the homepage and has been taken from the related shortcode. As an aside, we should see if we can templatize the search bar in the home page instead of putting it in every language's homepage. The whole page is also made a definition of the main block so that it can utilise the baseof.html layout.
  6. After the "search" layout was upated to use the search bar in the results page using the partial directly, the handling of the search bar for pagefind users broke. It was fixed by ensuring that the search bar was queried and deleted using a particular CSS id instead of a CSS class that could be used elsewhere.

The only visual changes I see are (left PR, right live)
image

  1. The search bar in the search page now has the same width as the home page
  2. The search results start with a "Search Results" heading. This is the default Docsy behaviour and if this is not desired, we can try to disable it using CSS.

I also plan to rework search-input.html and associated scss files but probably in a different PR.

Helps with #41171

@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 12, 2025
@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 Feb 12, 2025
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. labels Feb 12, 2025
@netlify
Copy link

netlify bot commented Feb 12, 2025

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit 8af9731
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/67d52b5ba50e040008ddb988
😎 Deploy Preview https://deploy-preview-49724--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
Copy link
Contributor

sftim commented Feb 12, 2025

As an aside, I was thinking of seeing if a "custom search" feature could be added in Docsy which would mean that we would just need to add a script and be done with any custom search functionality.

Suggest opening an upstream issue about the possibility.

@SayakMukhopadhyay
Copy link
Member Author

Suggest opening an upstream issue about the possibility.

google/docsy#2194 has been created. I will also note the same in the comments to ensure we don't forget to incorporate it if Docsy takes in the change.

@sftim
Copy link
Contributor

sftim commented Feb 23, 2025

Could we adopt google/docsy#1512 anyway, and use that behind the templates.Exists guard (disabling built-in search mechanisms and using our own complicated "you might be subject to state-level censorship" search functionality)?

@SayakMukhopadhyay
Copy link
Member Author

Could we adopt google/docsy#1512 anyway, and use that behind the templates.Exists guard (disabling built-in search mechanisms and using our own complicated "you might be subject to state-level censorship" search functionality)?

@sftim I have implemented something close to that PR, the only difference being that instead of a template.Exists check, I am using the .Site.Params.customSearch guard. I think its more transparent and less magic to have an explicit guard for a feature and not activating it by including a special file. Anyway, that param is also useful for including scripts and handling other customisations (see

{{ else if .Site.Params.customSearch }}
for eg.).

I have also mentioned this alternative in the mentioned PR in docsy.

@sftim
Copy link
Contributor

sftim commented Mar 5, 2025

(SGTM)

@SayakMukhopadhyay SayakMukhopadhyay marked this pull request as ready for review March 6, 2025 09:55
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2025
@SayakMukhopadhyay
Copy link
Member Author

This PR is ready for review. See the description for the details of the changes.

I have refrained from touching any stylistic changes as they might complicate the PR. I will be doing those in a separate PR.

Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

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

@SayakMukhopadhyay SayakMukhopadhyay changed the title Rework the search scripts to make it closer to docsy Rework the search scripts and layouts to make it closer to docsy Mar 7, 2025
@sftim
Copy link
Contributor

sftim commented Mar 9, 2025

/lgtm

@SayakMukhopadhyay I suggest you squash this to 1 commit, keeping the same merge base (if you keep the same final tree, the LGTM stays put).

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

LGTM label has been added.

Git tree hash: 96bf1a504f5094ded898e0f805b2291e04cf83b9

@SayakMukhopadhyay
Copy link
Member Author

SayakMukhopadhyay commented Mar 9, 2025

keeping the same merge base (if you keep the same final tree, the LGTM stays put).

Nice, I didn't know this.

@shurup
Copy link
Member

shurup commented Mar 12, 2025

Great job; LGTM, too! I checked how it works/renders in two browsers.

Copy link
Contributor

@chalin chalin left a comment

Choose a reason for hiding this comment

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

The K8s website isn't using any of the Docsy search functionality, right (i.e., you're not using GSE, nor Algolia, nor offline search)? If so, a simpler solution to what is proposed here is to just override layouts/partials/search-input.html (and assets/js/search.js) IMHO.

There is enough essential complexity in this website, we should strive to keep the design as simple as possible IMHO.

See inline comment for suggested code streamlining.


Edit

Oh, GCSE isn't being used here! I was confusing K8s with work on other sites were GCSE has been dropped (and was sure that that was the case for the K8s website).

Anyhow, my comment about simply overriding the two Docsy files seems to stand. I'm not sure what you gain by keeping the other Docsy code.

If you do override, and aren't sharing / reusing any of the code, what I've done in other projects is add a comment to the start of the file, something like this: https://github.com/open-telemetry/opentelemetry.io/blob/5b4881e6fc831d634ac635e3a70d916cafc17a31/assets/scss/_variables_project.scss#L1:

/* Docsy-delta full file override: we're not tracking changes to the Docsy file of the same name. */

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 14, 2025
@k8s-ci-robot k8s-ci-robot requested a review from sftim March 14, 2025 08:30
@SayakMukhopadhyay
Copy link
Member Author

Anyhow, my comment about simply overriding the two Docsy files seems to stand. I'm not sure what you gain by keeping the other Docsy code.

I have found that while migrating, having chunks of the Docsy code gives a good idea of how the custom code fits. For eg. when working with search-input.html, having the Docsy code means that when Docsy changes the css classes or the html structure in a certain way, I can do a quick 3 way diff between the previous Docsy layout, the current Docsy layout and the overridden layout to quickly figure out the exact changes I need to make.

This is especially true when Docsy introduces an enhancement or feature or removes a bug that we are implementing in the custom code. If we remove all Docsy code, even if they won't be executed either way, finding out if a piece of code is necessary after an upgrade becomes difficult.

Hence, I am trying to keep the overridden layouts as close to the Docsy layouts as possible, at least until we reach Docsy latest. Once that happens, I plan to add proper contribution documentation and comments on each override so that future upgrades can be simpler.

@sftim sftim requested a review from Copilot March 14, 2025 12:29
Copy link

Copilot AI left a 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 reworks the search functionality to align it more closely with Docsy by consolidating and updating search scripts and layouts. Key changes include:

  • Consolidation of search functionality into a new file (assets/js/custom-search.js) to simplify modifications.
  • Removal of deprecated search scripts and updates in the Hugo configuration (hugo.toml) to enable the new custom search.
  • Updates to the search UI layout to adopt Docsy’s styling and behavior.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

File Description
assets/js/custom-search.js New consolidated search script with updated logic and UI behavior
hugo.toml Updated configuration parameter to enable custom search
static/js/search.js Removed deprecated search script
assets/js/search.js Removed deprecated search script

feat: implement custom partial in search--input.html

chore: add some comments on how things can be refactored later

feat: bring the search.html closer to docsy by only including the search-input partial and using the other things from baseof.html

fix: first line comments need to be in the {{/* */}} block, see gohugoio/hugo#7243

Apply suggestions from code review

fix: apply review suggestions

fix: search bar should be removed in page find results and should be present in the sidebar

Co-authored-by: Tim Bannister <[email protected]>
Copy link
Member

@dipesh-rawat dipesh-rawat left a comment

Choose a reason for hiding this comment

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

/lgtm

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

LGTM label has been added.

Git tree hash: 844c36105c3bd865e0d1eefe3d5e6fca1115df42

@reylejano
Copy link
Member

Wonderful work!
Thank you for the improvements from your contribution @SayakMukhopadhyay
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: reylejano

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 Mar 18, 2025
@k8s-ci-robot k8s-ci-robot merged commit f067474 into kubernetes:main Mar 18, 2025
6 checks passed
@SayakMukhopadhyay SayakMukhopadhyay deleted the search-script-rework branch March 18, 2025 17:04
@sftim
Copy link
Contributor

sftim commented Mar 21, 2025

It was fixed by ensuring that the search bar was queried and deleted using a particular CSS id instead of a CSS class that could be used elsewhere.

BTW, watch out for cases where that element with a fixed id can appear twice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

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

Development

Successfully merging this pull request may close these issues.

7 participants