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

PageFind for users in China #46768

Merged
merged 4 commits into from
Jul 4, 2024
Merged

Conversation

cjyabraham
Copy link
Contributor

@cjyabraham cjyabraham commented Jun 11, 2024

First, this PR has PageFind search available for all users OUTSIDE of China. This is just temporary to facilitate testing and we will revert this before merging the PR. The goal is to show PageFind to all users IN China, as a replacement for Bing, which has been costing CNCF a lot of money recently.

Could someone help integrate this into the Makefile properly. I’m not sure where it should be applied and how to integrate it. You can see what I’ve done to get PageFind to run in the netlify.toml commands but that may not be the right place for them.

Searches with PageFind can be compared against the current site. If they are adequate, then we can get this deployed soon to remove Bing from the site. We should specifically test searches in Chinese but also test English and other languages.

We can enhance the PageFind search functionality in the future with filters, sorting, etc. If the PageFind results become better than those of Google Custom Search, we can have it serve all users, not just those in China.

I pulled ideas from #44655 when creating this PR but started from scratch as I couldn’t get that PR branch to run locally. I also kept the scope narrower and less ambitious, limiting PageFind search to just Chinese users while keeping Google Custom Search for everyone else. This was so that we could address the immediate need of getting off Bing as quickly as possible.

FYI this is the command I use to test this locally:

hugo && npx -y pagefind --site public --serve

@k8s-ci-robot k8s-ci-robot requested review from reylejano and sftim June 11, 2024 04:23
@k8s-ci-robot k8s-ci-robot added area/web-development Issues or PRs related to the kubernetes.io's infrastructure, design, or build processes sig/docs Categorizes an issue or PR as relevant to SIG Docs. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 11, 2024
Copy link

netlify bot commented Jun 11, 2024

Pull request preview available for checking

Built without sensitive environment variables

Name Link
🔨 Latest commit a4191d9
🔍 Latest deploy log https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/66856231fa04520008c48716
😎 Deploy Preview https://deploy-preview-46768--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.

@cjyabraham cjyabraham marked this pull request as draft June 11, 2024 06:08
@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 Jun 11, 2024
@cjyabraham cjyabraham marked this pull request as ready for review June 11, 2024 07:57
@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 Jun 11, 2024
@sftim
Copy link
Contributor

sftim commented Jun 11, 2024

Hi @cjyabraham

If I go to https://deploy-preview-46768--kubernetes-io-main-staging.netlify.app/zh-cn/docs/home/ and search for kubectl 命令行工具, I get pages in English. To be fair, I'm using a browser that prefers English but I was looking at the Chinese docs. This may well surprise users.

https://deploy-preview-46768--kubernetes-io-main-staging.netlify.app/search/?q=kubectl%20%E5%91%BD%E4%BB%A4%E8%A1%8C%E5%B7%A5%E5%85%B7

@sftim
Copy link
Contributor

sftim commented Jun 11, 2024

this PR has PageFind search available for all users OUTSIDE of China. This is just temporary to facilitate testing and we will revert this before merging the PR. The goal is to show PageFind to all users IN China, as a replacement for Bing, which has been #44475.

/hold
until this PR only shows PageFind to visitors from China

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 11, 2024
@cjyabraham
Copy link
Contributor Author

If I go to https://deploy-preview-46768--kubernetes-io-main-staging.netlify.app/zh-cn/docs/home/ and search for kubectl 命令行工具, I get pages in English. To be fair, I'm using a browser that prefers English but I was looking at the Chinese docs. This may well surprise users.

It looks like this is due to a problem with how the site currently works: if you're viewing Chinese pages and then perform a search, you always get redirected to the English search results page instead of staying on the Chinese search results page. I think once we fix this then the results will be pulled correctly from the Chinese content rather than the English. You can see how that should look on the Chinese search results page here: https://deploy-preview-46768--kubernetes-io-main-staging.netlify.app/zh-cn/search/?q=kubectl+%E5%91%BD%E4%BB%A4%E8%A1%8C%E5%B7%A5%E5%85%B7

@cjyabraham
Copy link
Contributor Author

Actually now that I think of it, it's a known issue with doscy which I have already submitted a fix for in this PR. Since that's not merged yet, we can reapply the fix locally within this site...

@cjyabraham
Copy link
Contributor Author

cjyabraham commented Jun 11, 2024

Ok, I've fixed that issue so now the site should correctly keep the user on the search page of the same language they were on. This is going to be a problem, however, for languages that don't have search pages, like French, so perhaps we should quickly add content files for them?

UPDATE: I've added the missing search.md content files.

@pacoxu
Copy link
Member

pacoxu commented Jun 11, 2024

/cc @windsonsea @tengqm

@k8s-ci-robot k8s-ci-robot added area/localization General issues or PRs related to localization language/de Issues or PRs related to German language language/es Issues or PRs related to Spanish language language/fr Issues or PRs related to French language language/hi Issues or PRs related to Hindi language language/it Issues or PRs related to Italian language language/ja Issues or PRs related to Japanese language language/ko Issues or PRs related to Korean language labels Jun 11, 2024
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
Signed-off-by: Chris Abraham <cjyabraham@gmail.com>
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2024
@k8s-ci-robot k8s-ci-robot requested review from sftim and ydFu July 3, 2024 14:37
@sftim
Copy link
Contributor

sftim commented Jul 3, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@sftim
Copy link
Contributor

sftim commented Jul 3, 2024

The PageFind functionality is only visible to users in China. However, you can check this out locally and try it by editing the search.js file; see https://github.com/kubernetes/website/compare/c86a1576dedaa56d1fce1ef0cbb7dda149477ba2..a4191d94ea52f8c4935c144f0d89d779b16f5812

@natalisucks
Copy link
Contributor

/hold

@kubernetes/steering-committee howdy steering! this change has come about out of the CNCF's need to replace Bing for users in China, and our zh-localization community, alongside other folks in SIG Docs, have reviewed this change to ensure it's doing what it describes. we want to check if this needs to be run by you before merging – thanks!

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@mrbobbytables
Copy link
Member

LGTM on my side, I don't think it needs steering sign off 👍

@BenTheElder
Copy link
Member

#46768 (comment) Agree.

@BenTheElder
Copy link
Member

Feel free to unhold, or wait for a simple majority of steering (routine business rules would apply if a vote were needed for something like this), but I pretty strongly think this does not need to wait for steering and is delegated to SIG Docs.

(re: delegation, maybe reach out to SIG K8s Infra if we have new paid SaaS accounts to manage but AFAICT this is not the case here and still wouldn't involve steering)

@natalisucks
Copy link
Contributor

sick, thanks @mrbobbytables and @BenTheElder!
/hold cancel
/lgtm

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 3, 2024
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 3, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 82c0ff4334fe8c9154b587542f3f9e454d0861fc

@nate-double-u
Copy link
Contributor

Thanks for this @cjyabraham & @sftim, and thanks to all the reviewers!

/approve

@sftim
Copy link
Contributor

sftim commented Jul 4, 2024

/lgtm

@sftim
Copy link
Contributor

sftim commented Jul 4, 2024

/assign natalisucks

@natalisucks
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: natalisucks, nate-double-u

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 Jul 4, 2024
@k8s-ci-robot k8s-ci-robot merged commit af3d53c into kubernetes:main Jul 4, 2024
6 checks passed
@cjyabraham cjyabraham deleted the pagefind2 branch July 4, 2024 08:01
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.